fix: remove db_owner requirement for allow_writes on MCP

[tsivaprasad]

Summary

This PR removes the validation that required connect_as to reference a db_owner: true user when allow_writes: true is set on an MCP service. PostgreSQL enforces write permissions at query time — the control plane check was redundant and blocked valid configurations.

Changes

• Remove db_owner enforcement in validateConnectAs() for MCP allow_writes
• Update test to assert no validation error for non-owner with allow_writes: true

Testing

Verification:
Before Fix:

curl -s -X POST http://localhost:3000/v1/databases \    
  -H "Content-Type: application/json" -d '{
  "id": "storefront",
  "spec": {
    "database_name": "storefront",
    "database_users": [
      {
        "username": "admin",
        "password": "password",
        "attributes": ["SUPERUSER", "LOGIN"]
      }
    ],
    "port": 0,
    "nodes": [
      { "name": "n1", "host_ids": ["host-1"] }
    ],
    "services": [
      {
        "service_id": "mcp-server",
        "service_type": "mcp",
        "version": "latest",
        "host_ids": ["host-1"],
        "port": 0,
        "connect_as": "admin",
        "config": {
          "allow_writes": true
        }
      }
    ]
  }
}'

{"name":"invalid_input","message":"services[0].connect_as: allow_writes requires connect_as to reference a database_users entry with db_owner: true"}

After Fix:

curl -s -X POST http://localhost:3000/v1/databases \
  -H "Content-Type: application/json" -d '{
  "id": "storefront",
  "spec": {
    "database_name": "storefront",
    "database_users": [
      {
        "username": "admin",
        "password": "password",
        "attributes": ["SUPERUSER", "LOGIN"]
      }
    ],
    "port": 0,
    "nodes": [
      { "name": "n1", "host_ids": ["host-1"] }
    ],
    "services": [
      {
        "service_id": "mcp-server",
        "service_type": "mcp",
        "version": "latest",
        "host_ids": ["host-1"],
        "port": 0,
        "connect_as": "admin",
        "config": {
          "allow_writes": true
        }
      }
    ]
  }
}'

{"task":{"scope":"database","entity_id":"storefront","database_id":"storefront","task_id":"019df438-da08-769a-a7e0-b30f14e28f2c","created_at":"2026-05-04T18:20:59Z","type":"create","status":"pending"},"database":{"id":"storefront","created_at":"2026-05-04T18:20:59Z","updated_at":"2026-05-04T18:20:59Z","state":"creating","spec":{"database_name":"storefront","postgres_version":"18.3","spock_version":"5","port":0,"nodes":[{"name":"n1","host_ids":["host-1"]}],"database_users":[{"username":"admin","db_owner":false,"attributes":["SUPERUSER","LOGIN"]}],"services":[{"service_id":"mcp-server","service_type":"mcp","version":"latest","host_ids":["host-1"],"port":0,"config":{"allow_writes":true},"connect_as":"admin"}]}}}

Checklist

• Tests added or updated

PLAT-587

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR removes validation logic that previously enforced db_owner: true for users referenced in connect_as when allow_writes is enabled. The check is deleted, allowing non-owner database users to be used with allow_writes.

Changes

Relaxed Connect-As Validation

Layer / File(s)	Summary
Core Validation Logic server/internal/api/apiv1/validate.go	Removed the check that rejected connect_as references to non-db_owner users when allow_writes: true. The function now only validates that the username matches an existing user.
Test Expectations server/internal/api/apiv1/validate_test.go	Updated TestValidateConnectAs "non-owner with allow_writes" case to expect an empty error list instead of a validation error about the db_owner requirement.

Poem

🐰 No more walls for writers bold,
Let non-owners' tales be told,
A hop toward freedom's way,
Validation's chains decay!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: removing the db_owner requirement for allow_writes on MCP services.
Description check	✅ Passed	The PR description covers all required sections: summary, changes, testing with before/after curl examples, issue link, and checklist acknowledgment of test updates.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch PLAT-587-remove-validation-requiring-db-owner-for-allow-writes

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

• Generate code and open pull requests
• Plan features and break down work
• Investigate incidents and troubleshoot customer tickets together
• Automate recurring tasks and respond to alerts with triggers
• Summarize progress and report instantly

Built for teams:

• Shared memory across your entire org—no repeating context
• Per-thread sandboxes to safely plan and execute work
• Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Review rate limit: 0/1 reviews remaining, refill in 60 minutes.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics -4 complexity · 0 duplication

Metric	Results
Complexity	-4
Duplication	0

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[coderabbitai]

🧹 Nitpick comments (1)

server/internal/api/apiv1/validate_test.go (1)

2101-2104: 💤 Low value

LGTM — assertion correctly reflects the removed db_owner enforcement.

The switch to assert.Empty(t, errs) aligns exactly with the updated validateConnectAs in validate.go (lines 409–423), which now returns nil immediately upon finding a matching username, without inspecting DbOwner or the allow_writes config key.

One optional note: the admin entry in the dbUsers fixture (line 2070) has DbOwner unset (nil) and is never directly exercised in a sub-test assertion. Adding a sub-test that uses admin with an explicit DbOwner: utils.PointerTo(false) and allow_writes: true would more precisely document the scenario described in the PR (a user with db_owner: false but SUPERUSER attributes), and make the fixture entry purposeful.

✨ Suggested additional sub-test

 	t.Run("non-owner with allow_writes", func(t *testing.T) {
 		errs := validateConnectAs(baseSvc("app_read_only", true), dbUsers, nil)
 		assert.Empty(t, errs)
 	})
+
+	t.Run("explicit db_owner=false user with allow_writes", func(t *testing.T) {
+		errs := validateConnectAs(baseSvc("admin", true), dbUsers, nil)
+		assert.Empty(t, errs)
+	})

And in the fixture, make the intent explicit:

-		{Username: "admin"},
+		{Username: "admin", DbOwner: utils.PointerTo(false)},

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/internal/api/apiv1/validate_test.go` around lines 2101 - 2104, The
test suite currently doesn't exercise the `admin` entry in the `dbUsers`
fixture; add a focused sub-test that calls validateConnectAs with the `admin`
user (from `dbUsers`) set to have DbOwner: utils.PointerTo(false) and run it
with allow_writes true, asserting the result is empty (assert.Empty(t, errs)) to
document the case "db_owner: false but SUPERUSER attributes" and make the
fixture purposeful; locate this change near the existing sub-test that uses
baseSvc("app_read_only", true) and the validateConnectAs call so the new
sub-test mirrors that pattern and clearly references `validateConnectAs`,
`dbUsers`, and the `admin` fixture entry.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@server/internal/api/apiv1/validate_test.go`:
- Around line 2101-2104: The test suite currently doesn't exercise the `admin`
entry in the `dbUsers` fixture; add a focused sub-test that calls
validateConnectAs with the `admin` user (from `dbUsers`) set to have DbOwner:
utils.PointerTo(false) and run it with allow_writes true, asserting the result
is empty (assert.Empty(t, errs)) to document the case "db_owner: false but
SUPERUSER attributes" and make the fixture purposeful; locate this change near
the existing sub-test that uses baseSvc("app_read_only", true) and the
validateConnectAs call so the new sub-test mirrors that pattern and clearly
references `validateConnectAs`, `dbUsers`, and the `admin` fixture entry.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8cc2f21e-3854-4ceb-b965-4bb7aa6a2501

📥 Commits

Reviewing files that changed from the base of the PR and between 76f2be6 and bce65a6.

📒 Files selected for processing (2)

• server/internal/api/apiv1/validate.go
• server/internal/api/apiv1/validate_test.go

💤 Files with no reviewable changes (1)

• server/internal/api/apiv1/validate.go

[rshoemaker]

LGTM!