diff --git a/CHANGELOG.md b/CHANGELOG.md index a7a3b3e..9db7adf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- A `create` tool, which creates a record by dispatching the resource's own + ActiveAdmin `create` action. Resources registered without that action are + refused, `permit_params` decides what may be written, the namespace's + authorization adapter is consulted before dispatch and again inside the + controller, and every ActiveAdmin callback (`before_build`, `before_create`, + `before_save`, …) fires. A record the model rejects comes back as a + `Validation failed` error carrying the model's own messages. + - 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 @@ -42,6 +50,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **Behaviour change:** `update` now dispatches the resource's real ActiveAdmin + `update` action instead of calling `record.update` directly. Everything the + admin UI runs on a save now runs on an MCP update too: the controller's + `before_action` chain, ActiveAdmin's `before_update` / `after_update` / + `before_save` / `after_save` callbacks, and the controller's own + authorization check. Applications whose callbacks have side effects — + auditing, notifications, derived columns, background jobs — will see those + fire for MCP updates where previously they were silently skipped. + + Two smaller consequences of the same change: a write rejected by the model + now reports `Validation failed` with the model's messages in `details` + rather than reporting whatever `record.update` returned, and the record + echoed back in the result has the same sensitive attributes stripped from it + (`encrypted_password`, `password_digest`, `reset_password_token`, `api_key`, + `secret`) that `list_resources` and `query` already omit. + - The release tag is now the source of truth for the bundle's version too: the release workflow writes it into `mcpb/manifest.json` and `mcpb/package.json` before packing, and commits the bump back alongside `version.rb`. The gem and @@ -56,6 +80,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 raising deliberately for ActiveAdmin 4. CI and the release workflow now run on Ruby 4.0.7. +### Removed + +- The fallback that derived writable fields from a resource's `form do ... end` + block when it declared no `permit_params`. Now that writes dispatch through + the real controller, ActiveAdmin resolves permitted params itself, and the + fallback turns out to have been granting MCP clients a write the admin UI + does not grant: a resource with no `permit_params` cannot be saved through + ActiveAdmin's own forms at all, because Rails raises + `ActiveModel::ForbiddenAttributesError` on the unpermitted params. Such a + resource is now refused with a message naming the missing `permit_params`, + rather than being written to. + ### Security - Enforce ActiveAdmin authorization on reads. `list_resources` and `query` diff --git a/README.md b/README.md index 6f04a97..5f8e47a 100644 --- a/README.md +++ b/README.md @@ -32,10 +32,12 @@ The server is a Rails engine mounted inside your application (by default at `scope_collection`, so the MCP user only ever sees the records they could see in the admin UI. With ActiveAdmin's default adapter every check passes, so applications without an authorization adapter are unaffected. -- **Writes go through ActiveAdmin.** The `update` tool only writes fields - allowed by the resource's `permit_params`, refuses resources that don't - register the `update` action, and runs every change through your - authorization adapter as the authenticated MCP user. +- **Writes go through ActiveAdmin.** The `create` and `update` tools dispatch + the resource's real ActiveAdmin controller action, so `permit_params`, your + `before_save`/`after_update` callbacks, the controller's `before_action` + chain and your authorization adapter all apply exactly as they do when + someone clicks Save in the admin UI. Resources that don't register the + action are refused. - **Authentication is optional but built in.** Enable Bearer-token auth and the installer adds an "MCP Tokens" management page to your ActiveAdmin panel. @@ -69,7 +71,8 @@ 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. | +| `create` | Create a new record through the resource's ActiveAdmin create action, honouring its permitted params, callbacks and authorization. | +| `update` | Update an existing record through the resource's ActiveAdmin update action, honouring its permitted params, callbacks 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 @@ -82,22 +85,36 @@ Find active posts created since the start of the month → query(resource: "Post", q: { status_eq: "active", created_at_gt: "2026-08-01" }) ``` -### Updating records +### Creating and updating records ``` +Create a user +→ create(resource: "User", attributes: { name: "Ada", email: "ada@example.com" }) + Update a user's name → update(resource: "User", id: 42, attributes: { name: "New name" }) ``` -The `update` tool applies the same rules as the ActiveAdmin UI: +Both tools dispatch the resource's own ActiveAdmin `create` or `update` +action, so a write from MCP is the same write the admin UI makes: -- **Editable resources only** — resources registered without the `update` - action (e.g. `actions :index, :show`) are refused. -- **Authorization** — the change runs through the resource namespace's - authorization adapter for the authenticated MCP user, so it can only update - what that user is allowed to update in admin. +- **Registered actions only** — resources registered without the action + (e.g. `actions :index, :show`) are refused. +- **Authorization** — the write runs through the resource namespace's + authorization adapter for the authenticated MCP user, both before dispatch + and again inside the controller, so it can only write what that user is + allowed to write in admin. - **Permitted fields only** — attributes are filtered through the resource's `permit_params`; fields the admin form doesn't accept are silently dropped. + A resource that declares no `permit_params` at all is refused outright, with + a message saying so — ActiveAdmin cannot write such a resource through its + own forms either. +- **Your callbacks run** — ActiveAdmin's `before_build`, `before_create`, + `before_save`, `after_update` and friends all fire, because the controller + action is what fires them. + +A write rejected by the model comes back as a `Validation failed` error with +the model's own messages in `details`, and nothing is written. ### Running member, collection and batch actions diff --git a/lib/activeadmin_mcp.rb b/lib/activeadmin_mcp.rb index 6ba5345..2bfe067 100644 --- a/lib/activeadmin_mcp.rb +++ b/lib/activeadmin_mcp.rb @@ -9,8 +9,7 @@ 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" +require_relative "activeadmin_mcp/record_writer" require_relative "activeadmin_mcp/request_handler" require_relative "activeadmin_mcp/engine" diff --git a/lib/activeadmin_mcp/controller_dispatcher.rb b/lib/activeadmin_mcp/controller_dispatcher.rb index d143c17..366b87b 100644 --- a/lib/activeadmin_mcp/controller_dispatcher.rb +++ b/lib/activeadmin_mcp/controller_dispatcher.rb @@ -14,6 +14,13 @@ def initialize(config:, current_user:) @current_user = current_user end + # Yields the controller once it has finished processing, so a caller that + # needs more than the redirect — the record a write built or loaded, and + # its validation errors — can read it off the controller itself. The block + # runs whether processing succeeded or raised, because a write that fails + # its validations re-renders the form, and that render is exactly the kind + # of thing a synthesized request can blow up on. Its return value is + # ignored and an exception inside it never reaches the caller. 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) @@ -21,15 +28,19 @@ def call(action:, path:, verb: :get, params: {}, path_params: {}) 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" } + begin + 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" } + ensure + inspect_controller(controller, action) { |processed| yield processed } if block_given? + end end # A controller instance for this resource with the MCP user injected, ready @@ -78,6 +89,12 @@ def current_user_methods ].select { |name| name.respond_to?(:to_sym) }.map(&:to_sym).uniq end + def inspect_controller(controller, action) + yield controller + rescue StandardError => e + warn("[activeadmin_mcp] inspecting #{@config.resource_class.name}##{action} raised #{e.class}: #{e.message}") + end + def build_request(path:, verb:, params:, action:, path_params:) env = Rack::MockRequest.env_for( path, diff --git a/lib/activeadmin_mcp/form_field_collector.rb b/lib/activeadmin_mcp/form_field_collector.rb deleted file mode 100644 index b3e27de..0000000 --- a/lib/activeadmin_mcp/form_field_collector.rb +++ /dev/null @@ -1,46 +0,0 @@ -module ActiveadminMcp - # Records the field names declared by an ActiveAdmin `form do ... end` block. - # - # ActiveAdmin form blocks are arbitrary Formtastic DSL — `input`, `inputs`, - # `actions`, helper calls, conditionals — so the block is run against this - # stand-in form builder. Every `input :field` records `:field`; every other - # message (including unknown helpers) is swallowed and returns self, so the - # block executes without a real view context. Nested `has_many` associations - # are intentionally not descended into: the updater only writes flat - # attributes, and descending would record association fields as top-level. - class FormFieldCollector - attr_reader :fields - - def initialize - @fields = [] - end - - def collect(&block) - instance_exec(self, &block) - @fields.uniq - end - - def input(name, *_args, **_opts, &_block) - @fields << name.to_sym if name.respond_to?(:to_sym) - self - end - - def inputs(*_args, **_opts, &block) - instance_exec(self, &block) if block - self - end - - def has_many(*_args, **_opts) - self - end - - def method_missing(_name, *_args, **_opts, &block) - instance_exec(self, &block) if block - self - end - - def respond_to_missing?(_name, _include_private = false) - true - end - end -end diff --git a/lib/activeadmin_mcp/record_updater.rb b/lib/activeadmin_mcp/record_updater.rb deleted file mode 100644 index 0a59332..0000000 --- a/lib/activeadmin_mcp/record_updater.rb +++ /dev/null @@ -1,106 +0,0 @@ -module ActiveadminMcp - # Updates a single ActiveAdmin-managed record, enforcing the same three gates - # the admin UI would: the resource must expose the update action, the current - # user must be authorized, and only fields the admin form permits are written. - class RecordUpdater - UPDATE = :update - - # Raised internally when the resource's permitted params cannot be resolved. - class PermitError < StandardError; end - - def initialize(resource:, current_user:) - @resource = resource - @current_user = current_user - end - - def call(id:, attributes:) - config = @resource[:config] - - return error("Resource is not editable: #{@resource[:name]}") unless editable?(config) - - record = @resource[:model].find_by(id: id) - return error("Record not found: #{@resource[:name]}##{id}") unless record - - unless authorized?(config, record) - return error("Not authorized to update #{@resource[:name]}##{id}") - end - - begin - permitted = permitted_attributes(config, attributes) - rescue PermitError => e - return error(e.message) - end - return error("No permitted attributes to update") if permitted.empty? - - if record.update(permitted) - { - resource: @resource[:name], - id: record.id, - updated: permitted.keys, - record: record.as_json, - } - else - error("Validation failed", details: record.errors.full_messages) - end - end - - private - - def editable?(config) - config.defined_actions.include?(UPDATE) - end - - def authorized?(config, record) - Authorization.for(config, @current_user).authorized?(UPDATE, record) - end - - # Resolves the fields we may write, accepting exactly what the admin form - # accepts. Prefers the resource's own `permit_params` (via the controller's - # compiled permitted_params); when a resource declares its writable fields - # through a `form do ... end` block instead — as ActiveAdmin's default - # permitted_params then returns nil — derives them from the form inputs. - # Fails closed if neither can be resolved. - def permitted_attributes(config, attributes) - from_permit_params(config, attributes) || - from_form(config, attributes) || - raise(PermitError, "Could not determine permitted attributes: #{@resource[:name]}") - end - - def from_permit_params(config, attributes) - param_key = config.param_key.to_sym - controller = config.controller.new - return nil unless controller.respond_to?(:permitted_params, true) - - controller.params = ActionController::Parameters.new(param_key => attributes) - permitted = controller.send(:permitted_params) - scoped = permitted && permitted[param_key] - scoped ? scoped.to_h.symbolize_keys : nil - rescue StandardError - nil - end - - def from_form(config, attributes) - fields = form_fields(config) - return nil if fields.empty? - - ActionController::Parameters.new(attributes).permit(*fields).to_h.symbolize_keys - rescue StandardError - nil - end - - def form_fields(config) - return [] unless config.respond_to?(:page_presenters) - - block = config.page_presenters[:form]&.block - return [] unless block - - FormFieldCollector.new.collect(&block) - end - - def error(message, details: nil) - result = { error: message } - result[:details] = details if details - result - end - end -end diff --git a/lib/activeadmin_mcp/record_writer.rb b/lib/activeadmin_mcp/record_writer.rb new file mode 100644 index 0000000..7ff7ce1 --- /dev/null +++ b/lib/activeadmin_mcp/record_writer.rb @@ -0,0 +1,161 @@ +module ActiveadminMcp + # Creates and updates ActiveAdmin-managed records by dispatching the + # resource's own create and update actions, so a write from an MCP client + # goes through the same permitted params, callbacks, before_action chain and + # authorization as the same write made by clicking Save in the admin UI. + # + # Three gates run before anything is dispatched: the resource must expose the + # action, the authorization adapter must allow it, and the resource must + # declare what may be written. The controller then applies all three again, + # against the record it builds or loads. + class RecordWriter + CREATE = :create + UPDATE = :update + + def initialize(resource:, current_user:) + @resource = resource + @current_user = current_user + @config = resource[:config] + end + + def create(attributes:) + return error("Resource is not creatable: #{resource_name}") unless exposes?(CREATE) + + permitted = resolve_permitted(attributes) + refusal = permit_refusal(permitted) + return refusal if refusal + return error("Not authorized to create #{resource_name}") unless authorized?(CREATE, @config.resource_class) + + write( + action: CREATE, + verb: :post, + path: @config.route_collection_path, + attributes: attributes, + written: permitted.keys, + written_key: :created, + description: "Create #{resource_name}" + ) + end + + def update(id:, attributes:) + return error("Resource is not editable: #{resource_name}") unless exposes?(UPDATE) + + record = @resource[:model].find_by(id: id) + return error("Record not found: #{resource_name}##{id}") unless record + + permitted = resolve_permitted(attributes) + refusal = permit_refusal(permitted) + return refusal if refusal + return error("Not authorized to update #{resource_name}##{id}") unless authorized?(UPDATE, record) + + write( + action: UPDATE, + verb: :patch, + path: @config.route_instance_path(record), + attributes: attributes, + written: permitted.keys, + written_key: :updated, + path_params: { id: record.to_param }, + description: "Update #{resource_name}##{id}" + ) + end + + private + + def write(action:, verb:, path:, attributes:, written:, written_key:, description:, path_params: {}) + record = nil + + outcome = ControllerDispatcher.new(config: @config, current_user: @current_user).call( + action: action, + verb: verb, + path: path, + params: { @config.param_key => attributes }, + path_params: path_params + ) { |controller| record = controller.send(:get_resource_ivar) } + + return rejection(record, description, outcome) unless saved?(record) + + { + resource: resource_name, + id: record.id, + written_key => written, + record: without_sensitive_attributes(record), + } + end + + def saved?(record) + !record.nil? && record.persisted? && record.errors.empty? + end + + # The controller declined to save. Validation messages are the usual + # reason, and they are read off the record rather than the response + # because a rejected write re-renders the admin form — a render the + # synthesized request may well not survive, leaving the dispatch itself + # reported as a failure even though the record knows exactly what was + # wrong with it. + # + # With no record and no validation messages, anything the controller put + # in the flash is the best account left of what the admin UI would have + # told the user. + def rejection(record, description, outcome) + messages = record ? record.errors.full_messages : [] + return error("Validation failed", details: messages) if messages.any? + return outcome if outcome[:error] + + error("#{description} failed", details: Array(outcome[:flash]&.values).presence) + end + + def exposes?(action) + @config.defined_actions.include?(action) + end + + def authorized?(action, subject) + Authorization.for(@config, @current_user).authorized?(action, subject) + end + + # Turns what the controller says it would permit into a refusal, or nil to + # go ahead. The answer is not used to filter the write — dispatch means + # ActiveAdmin applies permit_params itself — but to refuse early, and with + # a reason, in the two cases where dispatching would otherwise fail + # obscurely: a resource that never declared permit_params, and a call whose + # every attribute would be dropped. + # + # A resource with no permit_params cannot be written through ActiveAdmin's + # own forms either — Rails raises ForbiddenAttributesError on the + # unpermitted params — so refusing it says what the admin UI would. + def permit_refusal(permitted) + if permitted.nil? + return error("Resource declares no permit_params, so nothing may be written: #{resource_name}") + end + + error("No permitted attributes to write") if permitted.empty? + end + + # Asks the resource's own controller what it would permit, or nil when it + # has nothing to say because permit_params was never declared. + def resolve_permitted(attributes) + param_key = @config.param_key.to_sym + controller = @config.controller.new + controller.params = ActionController::Parameters.new(param_key => attributes) + permitted = controller.send(:permitted_params) + scoped = permitted && permitted[param_key] + scoped&.to_h&.symbolize_keys + rescue StandardError + nil + end + + def without_sensitive_attributes(record) + record.as_json.except(*ResourceRegistry.sensitive_attributes) + end + + def resource_name + @resource[:name] + end + + def error(message, details: nil) + result = { error: message } + result[:details] = details if details + result + end + end +end diff --git a/lib/activeadmin_mcp/request_handler.rb b/lib/activeadmin_mcp/request_handler.rb index c2993b0..c52e81f 100644 --- a/lib/activeadmin_mcp/request_handler.rb +++ b/lib/activeadmin_mcp/request_handler.rb @@ -114,10 +114,25 @@ def built_in_tools required: ["resource"], }, }, + { + name: "create", + description: "Create a new record. The write runs through the resource's real " \ + "ActiveAdmin create action, so only fields its permit_params accepts " \ + "are written and its callbacks and authorization all apply.", + inputSchema: { + type: "object", + properties: { + resource: { type: "string", description: "Resource name (e.g., 'User', 'Post')" }, + attributes: { type: "object", description: "Attributes for the new record (e.g., {name: 'Ada'})" }, + }, + required: %w[resource attributes], + }, + }, { name: "update", - description: "Update an existing record. Only fields the resource's ActiveAdmin " \ - "form permits are written, and the update respects ActiveAdmin authorization.", + description: "Update an existing record. The write runs through the resource's real " \ + "ActiveAdmin update action, so only fields its permit_params accepts " \ + "are written and its callbacks and authorization all apply.", inputSchema: { type: "object", properties: { @@ -138,6 +153,7 @@ def call_tool(params) result = case name when "list_resources" then tool_list_resources when "query" then tool_query(args) + when "create" then tool_create(args) when "update" then tool_update(args) else tool_action(name, args) end @@ -170,6 +186,16 @@ def tool_query(args) { resource: resource[:name], count: records.size, records: filter_sensitive(records.as_json) } end + def tool_create(args) + resource = ResourceRegistry.find(args["resource"]) + return { error: "Resource not found: #{args['resource']}" } unless resource + + attributes = args["attributes"] || {} + return { error: "attributes are required" } if attributes.empty? + + RecordWriter.new(resource: resource, current_user: @current_user).create(attributes: attributes) + end + def tool_update(args) resource = ResourceRegistry.find(args["resource"]) return { error: "Resource not found: #{args['resource']}" } unless resource @@ -178,8 +204,8 @@ def tool_update(args) attributes = args["attributes"] || {} return { error: "attributes are required" } if attributes.empty? - RecordUpdater.new(resource: resource, current_user: @current_user) - .call(id: args["id"], attributes: attributes) + RecordWriter.new(resource: resource, current_user: @current_user) + .update(id: args["id"], attributes: attributes) end def authorized_to_read?(resource) diff --git a/spec/activeadmin_mcp/controller_dispatcher_spec.rb b/spec/activeadmin_mcp/controller_dispatcher_spec.rb index 13ad460..bf73f77 100644 --- a/spec/activeadmin_mcp/controller_dispatcher_spec.rb +++ b/spec/activeadmin_mcp/controller_dispatcher_spec.rb @@ -1,15 +1,6 @@ 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") } @@ -20,13 +11,14 @@ def authorized?(_action, _subject = nil) AdminUser.delete_all end - def dispatch(action:, params: {}, path: nil, path_params: {}) + def dispatch(action:, params: {}, path: nil, path_params: {}, &block) 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) + path_params: { id: volunteer.id.to_s }.merge(path_params), + &block ) end @@ -156,4 +148,40 @@ def dispatch(action:, params: {}, path: nil, path_params: {}) expect(volunteer.reload.name).to eq("Ann") end end + + # Writes need more than the redirect: the caller has to read the record the + # controller built or loaded, and its validation errors, off the controller + # itself. The block is the seam that lets it. + describe "handing the processed controller back to the caller" do + it "yields the controller that processed the request" do + yielded = nil + + dispatch(action: :create_warning, params: { reason: "Late again" }) { |c| yielded = c } + + expect(yielded).to be_a(config.controller) + expect(yielded.send(:get_resource_ivar)).to eq(volunteer) + end + + # A failed write re-renders the form rather than redirecting, and that + # render can blow up in a synthesized request. The record carrying the + # validation errors must still reach the caller. + it "yields the controller even when processing raises" do + allow_any_instance_of(config.controller).to receive(:create_warning).and_raise("kaboom") + allow_any_instance_of(described_class).to receive(:warn) + yielded = nil + + result = dispatch(action: :create_warning, params: { reason: "Late" }) { |c| yielded = c } + + expect(yielded).to be_a(config.controller) + expect(result[:error]).to eq("Volunteer#create_warning failed") + end + + it "keeps a block that raises from destroying the result" do + allow_any_instance_of(described_class).to receive(:warn) + + result = dispatch(action: :create_warning, params: { reason: "Late again" }) { raise "from the block" } + + expect(result[:status]).to eq(302) + end + end end diff --git a/spec/activeadmin_mcp/record_updater_spec.rb b/spec/activeadmin_mcp/record_updater_spec.rb deleted file mode 100644 index 988e17d..0000000 --- a/spec/activeadmin_mcp/record_updater_spec.rb +++ /dev/null @@ -1,216 +0,0 @@ -require "spec_helper" - -RSpec.describe ActiveadminMcp::RecordUpdater do - # A stand-in for an ActiveAdmin controller compiled from `permit_params`. - # `permitted` is the set of fields the admin form would allow. - def build_controller(param_key:, permitted:) - Class.new do - attr_accessor :params - - define_method(:permitted_params) do - params.permit(param_key => permitted) - end - private :permitted_params - end - end - - def build_resource(model:, actions: %i[index show new create edit update destroy], - adapter:, param_key: :widget, permitted: %i[name role]) - namespace = double("namespace", authorization_adapter: adapter) - config = double( - "config", - defined_actions: actions, - namespace: namespace, - controller: build_controller(param_key: param_key, permitted: permitted), - param_key: param_key, - ) - { name: "Widget", model: model, config: config } - end - - def model_finding(record, id: 1) - double("model").tap { |m| allow(m).to receive(:find_by).with(id: id).and_return(record) } - end - - let(:permit_all) do - Class.new do - def initialize(*); end - def authorized?(*) = true - end - end - - let(:deny_all) do - Class.new do - def initialize(*); end - def authorized?(*) = false - end - end - - def update(resource, id: 1, attributes:, current_user: :admin) - described_class.new(resource: resource, current_user: current_user).call(id: id, attributes: attributes) - end - - it "writes permitted attributes and returns the updated record" do - record = double("record", id: 1, as_json: { "id" => 1, "name" => "Renamed" }) - allow(record).to receive(:update).and_return(true) - resource = build_resource(model: model_finding(record), adapter: permit_all) - - result = update(resource, attributes: { "name" => "Renamed" }) - - expect(record).to have_received(:update).with(name: "Renamed") - expect(result[:updated]).to eq([:name]) - expect(result[:record]).to eq("id" => 1, "name" => "Renamed") - end - - it "drops attributes the admin form does not permit" do - record = double("record", id: 1, as_json: {}) - allow(record).to receive(:update).and_return(true) - resource = build_resource(model: model_finding(record), adapter: permit_all, permitted: %i[name]) - - update(resource, attributes: { "name" => "Renamed", "role" => "admin" }) - - expect(record).to have_received(:update).with(name: "Renamed") - end - - it "refuses when ActiveAdmin does not expose the update action" do - resource = build_resource(model: double("model"), adapter: permit_all, actions: %i[index show]) - - result = update(resource, attributes: { "name" => "Renamed" }) - - expect(result[:error]).to match(/not editable/i) - end - - it "refuses when the user is not authorized to update the record" do - record = double("record", id: 1) - resource = build_resource(model: model_finding(record), adapter: deny_all) - - result = update(resource, attributes: { "name" => "Renamed" }) - - expect(result[:error]).to match(/not authorized/i) - end - - it "returns an error when the record does not exist" do - resource = build_resource(model: model_finding(nil, id: 999), adapter: permit_all) - - result = update(resource, id: 999, attributes: { "name" => "Renamed" }) - - expect(result[:error]).to match(/not found/i) - end - - it "returns validation errors when the update is rejected" do - record = double("record", id: 1, errors: double("errors", full_messages: ["Name can't be blank"])) - allow(record).to receive(:update).and_return(false) - resource = build_resource(model: model_finding(record), adapter: permit_all) - - result = update(resource, attributes: { "name" => "" }) - - expect(result[:error]).to match(/validation/i) - expect(result[:details]).to eq(["Name can't be blank"]) - end - - it "returns an error when no permitted attributes remain after filtering" do - record = double("record", id: 1) - resource = build_resource(model: model_finding(record), adapter: permit_all, permitted: %i[name]) - - result = update(resource, attributes: { "role" => "admin" }) - - expect(result[:error]).to match(/no permitted attributes/i) - end - - it "updates an attribute literally named 'error' without treating it as a failure" do - record = double("record", id: 1, as_json: { "id" => 1, "error" => "boom" }) - allow(record).to receive(:update).and_return(true) - resource = build_resource(model: model_finding(record), adapter: permit_all, permitted: %i[error]) - - result = update(resource, attributes: { "error" => "boom" }) - - expect(record).to have_received(:update).with(error: "boom") - expect(result[:updated]).to eq([:error]) - end - - # Resources that declare writable fields through a `form do ... end` block - # rather than `permit_params`. ActiveAdmin's default `permitted_params` for - # such a resource returns nil, so the updater derives the allowed fields from - # the form inputs instead. - describe "deriving permitted fields from the form block" do - # A controller that has not declared permit_params: `permitted_params` is nil. - def build_formless_controller - Class.new do - attr_accessor :params - def permitted_params = nil - private :permitted_params - end - end - - def build_form_resource(model:, adapter:, param_key: :widget, form_block:, - page_presenters: nil) - namespace = double("namespace", authorization_adapter: adapter) - presenters = page_presenters - presenters ||= { form: double("form presenter", block: form_block) } if form_block - config = double( - "config", - defined_actions: %i[index show new create edit update destroy], - namespace: namespace, - controller: build_formless_controller, - param_key: param_key, - page_presenters: presenters || {}, - ) - { name: "Widget", model: model, config: config } - end - - it "permits fields declared as form inputs and drops the rest" do - record = double("record", id: 1, as_json: {}) - allow(record).to receive(:update).and_return(true) - form_block = proc do |_f| - inputs do - input :name - input :role - end - actions - end - resource = build_form_resource(model: model_finding(record), adapter: permit_all, form_block: form_block) - - update(resource, attributes: { "name" => "Renamed", "role" => "admin", "secret" => "x" }) - - expect(record).to have_received(:update).with(name: "Renamed", role: "admin") - end - - it "tolerates arbitrary input options, helper calls and nesting in the form block" do - record = double("record", id: 1, as_json: {}) - allow(record).to receive(:update).and_return(true) - form_block = proc do |_f| - semantic_errors - inputs "Details" do - input :user_id, as: :hidden - input :name, as: :string, hint: some_undefined_helper - input :country, as: :select, collection: %w[UK IE] - end - actions - end - resource = build_form_resource(model: model_finding(record), adapter: permit_all, form_block: form_block) - - update(resource, attributes: { "user_id" => 5, "name" => "N", "country" => "UK", "nope" => 1 }) - - expect(record).to have_received(:update).with(user_id: 5, name: "N", country: "UK") - end - - it "fails closed when neither permit_params nor a form block is available" do - record = double("record", id: 1) - resource = build_form_resource(model: model_finding(record), adapter: permit_all, - form_block: nil, page_presenters: {}) - - result = update(resource, attributes: { "name" => "x" }) - - expect(result[:error]).to match(/could not determine permitted attributes/i) - end - - it "fails closed when the form block raises during introspection" do - record = double("record", id: 1) - form_block = proc { |_f| raise "boom" } - resource = build_form_resource(model: model_finding(record), adapter: permit_all, form_block: form_block) - - result = update(resource, attributes: { "name" => "x" }) - - expect(result[:error]).to match(/could not determine permitted attributes/i) - end - end -end diff --git a/spec/activeadmin_mcp/record_writer_spec.rb b/spec/activeadmin_mcp/record_writer_spec.rb new file mode 100644 index 0000000..ecb76d9 --- /dev/null +++ b/spec/activeadmin_mcp/record_writer_spec.rb @@ -0,0 +1,195 @@ +require "spec_helper" +require "support/active_admin" + +RSpec.describe ActiveadminMcp::RecordWriter do + let(:admin) { AdminUser.create!(email: "admin@example.com") } + let(:volunteers) { ActiveadminMcp::ResourceRegistry.find("Volunteer") } + let(:notes) { ActiveadminMcp::ResourceRegistry.find("Note") } + let(:sightings) { ActiveadminMcp::ResourceRegistry.find("Sighting") } + + after do + Volunteer.delete_all + Sighting.delete_all + AdminUser.delete_all + end + + def writer(resource) + described_class.new(resource: resource, current_user: admin) + end + + describe "creating a record" do + it "creates the record and hands back its id and attributes" do + result = writer(volunteers).create(attributes: { "name" => "Ann", "active" => false }) + + expect(result[:error]).to be_nil + expect(result[:record]).to include("name" => "Ann", "active" => false) + expect(Volunteer.find(result[:id]).name).to eq("Ann") + end + + it "reports which of the submitted attributes the admin form permitted" do + result = writer(volunteers).create(attributes: { "name" => "Ann" }) + + expect(result[:created]).to eq([:name]) + end + + it "runs ActiveAdmin's own callbacks, which only fire through the controller" do + result = writer(volunteers).create(attributes: { "name" => " Ann " }) + + expect(Volunteer.find(result[:id]).name).to eq("Ann") + end + + it "drops an attribute the resource's permit_params does not accept" do + result = writer(volunteers).create(attributes: { "name" => "Ann", "id" => 4321 }) + + expect(result[:id]).not_to eq(4321) + expect(Volunteer.exists?(4321)).to be(false) + end + + # A rejected write re-renders the admin form, which the synthesized + # request does not survive here; the dispatcher logs that and the writer + # falls back to the record's own errors, which is the point of the example. + it "returns the validation messages when the record is rejected" do + allow_any_instance_of(ActiveadminMcp::ControllerDispatcher).to receive(:warn) + + result = writer(volunteers).create(attributes: { "name" => "" }) + + expect(result[:error]).to match(/validation/i) + expect(result[:details]).to include("Name can't be blank") + expect(Volunteer.count).to eq(0) + end + + it "refuses a resource registered without the create action" do + result = writer(notes).create(attributes: { "body" => "Nope" }) + + expect(result[:error]).to match(/not creatable/i) + expect(Note.count).to eq(0) + end + + it "refuses a resource that declares no permitted params" do + result = writer(sightings).create(attributes: { "species" => "Kestrel" }) + + expect(result[:error]).to match(/permit_params/) + expect(Sighting.count).to eq(0) + end + + it "refuses when none of the submitted attributes are permitted" do + result = writer(volunteers).create(attributes: { "nonsense" => 1 }) + + expect(result[:error]).to match(/no permitted attributes/i) + expect(Volunteer.count).to eq(0) + end + + it "omits sensitive attributes from the record it returns" do + allow(ActiveadminMcp::ResourceRegistry).to receive(:sensitive_attributes).and_return(%w[name]) + + result = writer(volunteers).create(attributes: { "name" => "Ann" }) + + expect(result[:record]).not_to have_key("name") + end + end + + describe "updating a record" do + let!(:volunteer) { Volunteer.create!(name: "Ann", active: true) } + + it "writes the permitted attributes and hands back the updated record" do + result = writer(volunteers).update(id: volunteer.id, attributes: { "name" => "Bea" }) + + expect(result[:error]).to be_nil + expect(result[:updated]).to eq([:name]) + expect(result[:record]).to include("name" => "Bea") + expect(volunteer.reload.name).to eq("Bea") + end + + it "runs ActiveAdmin's own callbacks, which only fire through the controller" do + writer(volunteers).update(id: volunteer.id, attributes: { "name" => " Bea " }) + + expect(volunteer.reload.name).to eq("Bea") + end + + it "drops an attribute the resource's permit_params does not accept" do + writer(volunteers).update(id: volunteer.id, attributes: { "name" => "Bea", "created_at" => "1999-01-01" }) + + expect(volunteer.reload.name).to eq("Bea") + expect(volunteer.created_at.year).not_to eq(1999) + end + + it "returns the validation messages and leaves the record alone when rejected" do + allow_any_instance_of(ActiveadminMcp::ControllerDispatcher).to receive(:warn) + + result = writer(volunteers).update(id: volunteer.id, attributes: { "name" => "" }) + + expect(result[:error]).to match(/validation/i) + expect(result[:details]).to include("Name can't be blank") + expect(volunteer.reload.name).to eq("Ann") + end + + it "refuses a resource registered without the update action" do + note = Note.create!(body: "Untouched") + + result = writer(notes).update(id: note.id, attributes: { "body" => "Tampered" }) + + expect(result[:error]).to match(/not editable/i) + expect(note.reload.body).to eq("Untouched") + ensure + Note.delete_all + end + + it "refuses a resource that declares no permitted params" do + sighting = Sighting.create!(species: "Kestrel") + + result = writer(sightings).update(id: sighting.id, attributes: { "species" => "Merlin" }) + + expect(result[:error]).to match(/permit_params/) + expect(sighting.reload.species).to eq("Kestrel") + end + + it "refuses when none of the submitted attributes are permitted" do + result = writer(volunteers).update(id: volunteer.id, attributes: { "nonsense" => 1 }) + + expect(result[:error]).to match(/no permitted attributes/i) + expect(volunteer.reload.name).to eq("Ann") + end + + it "reports a record that does not exist rather than raising" do + result = writer(volunteers).update(id: 999_999, attributes: { "name" => "Bea" }) + + expect(result[:error]).to eq("Record not found: Volunteer#999999") + end + + it "omits sensitive attributes from the record it returns" do + allow(ActiveadminMcp::ResourceRegistry).to receive(:sensitive_attributes).and_return(%w[name]) + + result = writer(volunteers).update(id: volunteer.id, attributes: { "name" => "Bea" }) + + expect(result[:record]).not_to have_key("name") + end + end + + # Neutralising the namespace's authentication_method must not neutralise + # authorization: a user the adapter refuses must not be able to write. + describe "when the authorization adapter denies everything" 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 "refuses to create the record" do + result = writer(volunteers).create(attributes: { "name" => "Ann" }) + + expect(result[:error]).to match(/not authorized/i) + expect(Volunteer.count).to eq(0) + end + + it "refuses to update the record" do + volunteer = Volunteer.create!(name: "Ann") + + result = writer(volunteers).update(id: volunteer.id, attributes: { "name" => "Bea" }) + + expect(result[:error]).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 a18a1cd..238533e 100644 --- a/spec/activeadmin_mcp/request_handler_spec.rb +++ b/spec/activeadmin_mcp/request_handler_spec.rb @@ -58,10 +58,10 @@ def handle(method, params = nil, id: 1) describe "tools/list" do before { allow(ActiveadminMcp::ActionCatalog).to receive(:all).and_return([]) } - it "advertises the list_resources, query and update tools" do + it "advertises the list_resources, query, create and update tools" do tools = handle("tools/list")[:result][:tools] - expect(tools.map { |t| t[:name] }).to contain_exactly("list_resources", "query", "update") + expect(tools.map { |t| t[:name] }).to contain_exactly("list_resources", "query", "create", "update") end it "marks resource as required on the query tool" do @@ -77,6 +77,13 @@ def handle(method, params = nil, id: 1) expect(update[:inputSchema][:required]).to contain_exactly("resource", "id", "attributes") end + + it "requires resource and attributes on the create tool" do + tools = handle("tools/list")[:result][:tools] + create = tools.find { |t| t[:name] == "create" } + + expect(create[:inputSchema][:required]).to contain_exactly("resource", "attributes") + end end describe "unknown method" do @@ -96,6 +103,15 @@ def call_tool(name, arguments = {}) JSON.parse(text) end + def call_tool_as(current_user, name, arguments = {}) + response = described_class.new(current_user: current_user).handle( + "id" => 1, + "method" => "tools/call", + "params" => { "name" => name, "arguments" => arguments }, + ) + JSON.parse(response[:result][:content].first[:text]) + end + def resource_config(authorized: true) namespace = double("namespace", authorization_adapter: adapter_class(authorized: authorized)) double("config", namespace: namespace) @@ -212,30 +228,52 @@ def stub_resource(authorized: true) .to eq("error" => "attributes are required") end - it "delegates to the record updater with the resource and current user" do + it "delegates to the record writer with the resource and current user" do resource = { name: "User", model: double, config: double } allow(ActiveadminMcp::ResourceRegistry).to receive(:find).with("User").and_return(resource) - updater = instance_double(ActiveadminMcp::RecordUpdater, call: { updated: [:name] }) - allow(ActiveadminMcp::RecordUpdater).to receive(:new).and_return(updater) - - handler = described_class.new(current_user: :admin) - response = handler.handle( - "id" => 1, - "method" => "tools/call", - "params" => { - "name" => "update", - "arguments" => { "resource" => "User", "id" => 7, "attributes" => { "name" => "x" } }, - }, - ) - result = JSON.parse(response[:result][:content].first[:text]) + writer = instance_double(ActiveadminMcp::RecordWriter, update: { updated: [:name] }) + allow(ActiveadminMcp::RecordWriter).to receive(:new).and_return(writer) - expect(ActiveadminMcp::RecordUpdater).to have_received(:new) + result = call_tool_as(:admin, "update", "resource" => "User", "id" => 7, "attributes" => { "name" => "x" }) + + expect(ActiveadminMcp::RecordWriter).to have_received(:new) .with(resource: resource, current_user: :admin) - expect(updater).to have_received(:call).with(id: 7, attributes: { "name" => "x" }) + expect(writer).to have_received(:update).with(id: 7, attributes: { "name" => "x" }) expect(result).to eq("updated" => ["name"]) end end + describe "create" do + it "returns an error when the resource is not found" do + allow(ActiveadminMcp::ResourceRegistry).to receive(:find).with("Ghost").and_return(nil) + + expect(call_tool("create", "resource" => "Ghost", "attributes" => { "name" => "x" })) + .to eq("error" => "Resource not found: Ghost") + end + + it "returns an error when no attributes are given" do + allow(ActiveadminMcp::ResourceRegistry).to receive(:find) + .with("User").and_return(name: "User", model: double, config: double) + + expect(call_tool("create", "resource" => "User")) + .to eq("error" => "attributes are required") + end + + it "delegates to the record writer with the resource and current user" do + resource = { name: "User", model: double, config: double } + allow(ActiveadminMcp::ResourceRegistry).to receive(:find).with("User").and_return(resource) + writer = instance_double(ActiveadminMcp::RecordWriter, create: { id: 7 }) + allow(ActiveadminMcp::RecordWriter).to receive(:new).and_return(writer) + + result = call_tool_as(:admin, "create", "resource" => "User", "attributes" => { "name" => "x" }) + + expect(ActiveadminMcp::RecordWriter).to have_received(:new) + .with(resource: resource, current_user: :admin) + expect(writer).to have_received(:create).with(attributes: { "name" => "x" }) + expect(result).to eq("id" => 7) + end + end + describe "an unknown tool" do it "returns an error naming the tool" do expect(call_tool("frobnicate")).to eq("error" => "Unknown tool: frobnicate") diff --git a/spec/e2e/fixture_app/app/admin/posts.rb b/spec/e2e/fixture_app/app/admin/posts.rb index ae47914..fff3c01 100644 --- a/spec/e2e/fixture_app/app/admin/posts.rb +++ b/spec/e2e/fixture_app/app/admin/posts.rb @@ -3,6 +3,14 @@ ActiveAdmin.register Post do permit_params :title, :body + # These ActiveAdmin callbacks fire only when the create or update action runs + # through the real controller, so the e2e suite can read their effects to + # prove an MCP write is dispatched rather than written straight to the model. + # `slug` is deliberately outside `permit_params`, so a created post can only + # get one from this callback. + before_create { |post| post.slug = post.title.to_s.parameterize if post.slug.blank? } + before_update { |post| post.body = "#{post.body} (revised)" } + # 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 diff --git a/spec/e2e/fixture_app/app/admin/tags.rb b/spec/e2e/fixture_app/app/admin/tags.rb new file mode 100644 index 0000000..d9c7979 --- /dev/null +++ b/spec/e2e/fixture_app/app/admin/tags.rb @@ -0,0 +1,6 @@ +# Registered with ActiveAdmin's default actions but deliberately without +# `permit_params`, so the e2e suite can prove the MCP `create` and `update` +# tools refuse a resource that has never declared what may be written — which +# is a resource ActiveAdmin cannot write through its own forms either. +ActiveAdmin.register Tag do +end diff --git a/spec/e2e/fixture_app/app/models/post.rb b/spec/e2e/fixture_app/app/models/post.rb index 6003a1f..fe4cd17 100644 --- a/spec/e2e/fixture_app/app/models/post.rb +++ b/spec/e2e/fixture_app/app/models/post.rb @@ -1,4 +1,6 @@ class Post < ApplicationRecord + validates :title, presence: true + # Ransack 4 refuses to filter on any attribute absent from this allowlist, # and the MCP `query` tool calls `ransack` directly rather than going # through an ActiveAdmin filter. diff --git a/spec/e2e/fixture_app/app/models/tag.rb b/spec/e2e/fixture_app/app/models/tag.rb new file mode 100644 index 0000000..c23f76f --- /dev/null +++ b/spec/e2e/fixture_app/app/models/tag.rb @@ -0,0 +1,6 @@ +class Tag < ApplicationRecord + # See the note in post.rb: Ransack 4 requires an explicit allowlist. + def self.ransackable_attributes(_auth_object = nil) + column_names + end +end diff --git a/spec/e2e/fixture_app/db/migrate/20260101000004_create_tags.rb b/spec/e2e/fixture_app/db/migrate/20260101000004_create_tags.rb new file mode 100644 index 0000000..57c83f7 --- /dev/null +++ b/spec/e2e/fixture_app/db/migrate/20260101000004_create_tags.rb @@ -0,0 +1,9 @@ +class CreateTags < ActiveRecord::Migration[7.2] + def change + create_table :tags do |t| + t.string :name + + t.timestamps + end + end +end diff --git a/spec/e2e/fixture_app/db/seeds.rb b/spec/e2e/fixture_app/db/seeds.rb index b5620f1..d942e22 100644 --- a/spec/e2e/fixture_app/db/seeds.rb +++ b/spec/e2e/fixture_app/db/seeds.rb @@ -36,3 +36,8 @@ post.save! end end + +# A resource registered without `permit_params`, so the write tools have +# something to refuse. Nothing mutates it: the examples that name it assert it +# is unchanged. +Tag.find_or_initialize_by(name: "fantasy").save! diff --git a/spec/e2e/mcp_server_spec.rb b/spec/e2e/mcp_server_spec.rb index 9798e70..7bf3d5b 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 built-in tools, alongside whatever the fixture app has opted in to MCP" do + it "advertises the four 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 include("list_resources", "query", "update") + expect(names).to include("list_resources", "query", "create", "update") end end diff --git a/spec/e2e/mcp_tools_spec.rb b/spec/e2e/mcp_tools_spec.rb index fe49afd..d8e2a3e 100644 --- a/spec/e2e/mcp_tools_spec.rb +++ b/spec/e2e/mcp_tools_spec.rb @@ -58,6 +58,60 @@ end end + describe "create" do + def posts + client.call_tool("query", resource: "Post")["records"] + end + + it "creates a record through the resource's own ActiveAdmin create action, so its callbacks run" do + result = client.call_tool("create", resource: "Post", attributes: { title: "Mort", body: "The fourth." }) + + expect(result["error"]).to be_nil + expect(result["created"]).to contain_exactly("title", "body") + + created = client.call_tool("query", resource: "Post", q: { id_eq: result["id"] })["records"].first + expect(created["title"]).to eq("Mort") + expect(created["slug"]).to eq("mort") + end + + it "drops an attribute the resource's permitted params do not accept" do + result = client.call_tool( + "create", + resource: "Post", + attributes: { title: "Mort", slug: "tampered" } + ) + + expect(result["created"]).to eq(["title"]) + + created = client.call_tool("query", resource: "Post", q: { id_eq: result["id"] })["records"].first + expect(created["slug"]).to eq("mort") + end + + it "reports the model's validation messages and creates nothing when the new record is rejected" do + result = client.call_tool("create", resource: "Post", attributes: { title: "", body: "No title." }) + + expect(result["error"]).to match(/validation/i) + expect(result["details"]).to include("Title can't be blank") + expect(posts.length).to eq(3) + end + + it "refuses to create a record for a resource registered without the create action" do + result = client.call_tool("create", resource: "Author", attributes: { name: "Iain" }) + + expect(result["error"]).to eq("Resource is not creatable: Author") + expect(client.call_tool("query", resource: "Author")["count"]).to eq(2) + end + + it "refuses to create a record for a resource that declares no permitted params" do + result = client.call_tool("create", resource: "Tag", attributes: { name: "science-fiction" }) + + expect(result["error"]).to match(/permit_params/) + + tags = client.call_tool("query", resource: "Tag")["records"] + expect(tags.map { |tag| tag["name"] }).to eq(["fantasy"]) + end + end + describe "update" do let(:post_id) do client.call_tool("query", resource: "Post", q: { slug_eq: "small-gods" })["records"].first["id"] @@ -88,6 +142,34 @@ expect(reread["slug"]).to eq("small-gods") end + it "updates through the resource's own ActiveAdmin update action, so its callbacks run" do + client.call_tool("update", resource: "Post", id: post_id, attributes: { title: "Pyramids" }) + + reread = client.call_tool("query", resource: "Post", q: { id_eq: post_id })["records"].first + expect(reread["body"]).to eq("Unrelated. (revised)") + end + + it "reports the model's validation messages and leaves the record alone when the change is rejected" do + result = client.call_tool("update", resource: "Post", id: post_id, attributes: { title: "" }) + + expect(result["error"]).to match(/validation/i) + expect(result["details"]).to include("Title can't be blank") + + reread = client.call_tool("query", resource: "Post", q: { id_eq: post_id })["records"].first + expect(reread["title"]).to eq("Small Gods") + end + + it "refuses to update a resource that declares no permitted params" do + tag_id = client.call_tool("query", resource: "Tag")["records"].first["id"] + + result = client.call_tool("update", resource: "Tag", id: tag_id, attributes: { name: "tampered" }) + + expect(result["error"]).to match(/permit_params/) + + reread = client.call_tool("query", resource: "Tag", q: { id_eq: tag_id })["records"].first + expect(reread["name"]).to eq("fantasy") + end + it "refuses a resource that does not register the update action" do author_id = client.call_tool("query", resource: "Author", q: { name_eq: "Terry" })["records"].first["id"] diff --git a/spec/support/active_admin.rb b/spec/support/active_admin.rb index e6fb49e..08ad1d1 100644 --- a/spec/support/active_admin.rb +++ b/spec/support/active_admin.rb @@ -22,13 +22,31 @@ t.timestamps end + # Registered read-only, so a spec can prove a write is refused for a + # resource whose ActiveAdmin registration does not expose the action. + create_table :notes, force: true do |t| + t.string :body + t.timestamps + end + + # Registered writable but WITHOUT permit_params, so a spec can prove a write + # is refused for a resource that never declared what may be written. + create_table :sightings, force: true do |t| + t.string :species + t.timestamps + end + create_table :admin_users, force: true do |t| t.string :email t.timestamps end end -class Volunteer < ActiveRecord::Base; end +class Volunteer < ActiveRecord::Base + validates :name, presence: true +end +class Note < ActiveRecord::Base; end +class Sighting < ActiveRecord::Base; end class AdminUser < ActiveRecord::Base; end require "active_admin" @@ -67,7 +85,14 @@ class ApplicationController < ActionController::Base; end ActiveAdmin.application.namespaces[:admin].batch_actions = true ActiveAdmin.register Volunteer do - actions :index, :show, :edit, :update + actions :index, :show, :new, :create, :edit, :update + + permit_params :name, :active + + # An ActiveAdmin callback fires only when the create or update action runs + # through the real controller, so a spec can use the trimmed name to prove a + # write was dispatched rather than written straight to the model. + before_save { |volunteer| volunteer.name = volunteer.name.to_s.strip } member_action :create_warning, method: :post, mcp: { description: "Record a warning against a volunteer", @@ -92,6 +117,23 @@ class ApplicationController < ActionController::Base; end end end +ActiveAdmin.register Note do + actions :index, :show +end + +ActiveAdmin.register Sighting do +end + +# An authorization adapter that denies everything, used to prove that +# neutralising the namespace's authentication_method (see +# ControllerDispatcher#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 + Rails.application.routes.draw do ActiveAdmin.routes(self) end