Align MCP create_record/update_record with shared authorization helper - #3730
Align MCP create_record/update_record with shared authorization helper#3730souvikghosh04 wants to merge 5 commits into
Conversation
…r pattern Extends McpAuthorizationHelper with a small reusable check used by the read-side MCP tools, and calls it from CreateRecordTool/UpdateRecordTool for consistency with the rest of the tool set. Adds accompanying test coverage.
There was a problem hiding this comment.
Pull request overview
This PR tightens MCP DML tool authorization by adding an explicit column-level authorization check for create_record and update_record, aligning these tools with existing authorization patterns used elsewhere (and preventing fields.exclude/fields.include bypasses).
Changes:
- Added a reusable column-level authorization helper (
McpAuthorizationHelper.AreColumnsAuthorizedForOperation). - Updated
CreateRecordToolandUpdateRecordToolto enforce column-level authorization immediately after role resolution. - Added a dedicated test role/config entry plus regression tests to validate excluded-column enforcement for both create and update.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Azure.DataApiBuilder.Mcp/Utils/McpAuthorizationHelper.cs | Adds a shared helper to enforce column-level authorization for mutations. |
| src/Azure.DataApiBuilder.Mcp/BuiltInTools/CreateRecordTool.cs | Enforces column-level authorization for create payload columns. |
| src/Azure.DataApiBuilder.Mcp/BuiltInTools/UpdateRecordTool.cs | Enforces column-level authorization for update payload columns. |
| src/Service.Tests/Mcp/McpToolTestBase.cs | Enables mutation test service providers to simulate different client roles. |
| src/Service.Tests/Mcp/CreateRecordToolMsSqlIntegrationTests.cs | Adds regression + sanity tests for excluded-column behavior on create. |
| src/Service.Tests/Mcp/UpdateRecordToolMsSqlIntegrationTests.cs | Adds regression + sanity tests for excluded-column behavior on update. |
| src/Service.Tests/dab-config.MsSql.json | Adds a test role with mutation-time excluded fields for the new regression tests. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
… test The Book entity's only non-key column (publisher_id) is NOT NULL with no default, so a role excluded from writing it can never satisfy Book's required-field validation on create -- unrelated to authorization. Switched the success-path sanity check to the WebsiteUser entity, whose only non-key column (username) is nullable, so omitting the excluded column is valid.
c84ec2c to
fb815cd
Compare
The MCP create_record/update_record column-exclusion integration tests passed locally but failed in CI. CI regenerates src/Service.Tests/dab-config.MsSql.json from config-generators/mssql-commands.txt; the role existed only in the checked-in JSON, so the regenerated CI config lacked it and inheritance from the 'authenticated' role (no field exclusions) allowed writes to excluded columns. Add the role to mssql-commands.txt for Book (exclude publisher_id) and WebsiteUser (exclude username) on create/update. Add AuthorizationResolver unit tests covering multi-action-per-role column exclusion and config parsing.
fb815cd to
20d95f7
Compare
RubenCerna2079
left a comment
There was a problem hiding this comment.
Just a few changes that need to be addressed
|
|
||
| // Column-level authorization: ensure the caller's effective role is permitted to write | ||
| // every column present in the request payload (fields.include/fields.exclude enforcement). | ||
| if (!McpAuthorizationHelper.AreColumnsAuthorizedForOperation( |
There was a problem hiding this comment.
Why is the create under a try catch while this one isn't? Is there a reason this one shouldn't need it?
| { | ||
| error = string.Empty; | ||
|
|
||
| List<string> requestedColumns = columns is null ? new List<string>() : columns.ToList(); |
There was a problem hiding this comment.
I don't think it is possible for the columns variable to be null, at most empty. So this else if would be a bit irrelevant wouldn't it?
| Actions: new EntityAction[] { readAction, createAction, updateAction, deleteAction }); | ||
|
|
||
| Azure.DataApiBuilder.Config.ObjectModel.Entity sampleEntity = new( | ||
| Source: new Azure.DataApiBuilder.Config.ObjectModel.EntitySource(AuthorizationHelpers.TEST_ENTITY, Azure.DataApiBuilder.Config.ObjectModel.EntitySourceType.Table, null, null), |
There was a problem hiding this comment.
nit: Couldn't we add this as libraries instead of putting the entire namespace here?
| Assert.IsTrue(authZResolver.AreColumnsAllowedForOperation( | ||
| AuthorizationHelpers.TEST_ENTITY, | ||
| AuthorizationHelpers.TEST_ROLE, | ||
| operation: EntityActionOperation.Read, | ||
| new List<string> { "col1", "col3" })); | ||
|
|
||
| // Create should DENY col3 since it is excluded for create. | ||
| Assert.IsFalse(authZResolver.AreColumnsAllowedForOperation( | ||
| AuthorizationHelpers.TEST_ENTITY, | ||
| AuthorizationHelpers.TEST_ROLE, | ||
| operation: EntityActionOperation.Create, | ||
| new List<string> { "col1", "col3" })); | ||
|
|
||
| // Update should DENY col3 since it is excluded for update. | ||
| Assert.IsFalse(authZResolver.AreColumnsAllowedForOperation( | ||
| AuthorizationHelpers.TEST_ENTITY, | ||
| AuthorizationHelpers.TEST_ROLE, | ||
| operation: EntityActionOperation.Update, | ||
| new List<string> { "col1", "col3" })); |
There was a problem hiding this comment.
nit: I think we should add a message for when either of these asserts fail to make it easier to debug
| Assert.IsTrue(roundTrippedResolver.AreColumnsAllowedForOperation( | ||
| AuthorizationHelpers.TEST_ENTITY, | ||
| AuthorizationHelpers.TEST_ROLE, | ||
| operation: EntityActionOperation.Read, | ||
| new List<string> { "col1", "col3" })); | ||
|
|
||
| Assert.IsFalse(roundTrippedResolver.AreColumnsAllowedForOperation( | ||
| AuthorizationHelpers.TEST_ENTITY, | ||
| AuthorizationHelpers.TEST_ROLE, | ||
| operation: EntityActionOperation.Create, | ||
| new List<string> { "col1", "col3" })); | ||
|
|
||
| Assert.IsFalse(roundTrippedResolver.AreColumnsAllowedForOperation( | ||
| AuthorizationHelpers.TEST_ENTITY, | ||
| AuthorizationHelpers.TEST_ROLE, | ||
| operation: EntityActionOperation.Update, | ||
| new List<string> { "col1", "col3" })); |
There was a problem hiding this comment.
Same as previous comment
Summary
Minor internal consistency update for the built-in MCP DML tools (create_record, update_record).
Both tools now route through a small shared authorization helper (McpAuthorizationHelper) before invoking the mutation engine, matching the pattern already used elsewhere in the MCP tool set. No changes to public tool schemas, request/response shapes, or supported scenarios.
Changes
Testing
Notes
Opened as draft while final validation completes.