From e4d4d8e4dcd800c1102c0dfc9a18617f6ddb7215 Mon Sep 17 00:00:00 2001
From: Mattsson <111893710+mattssonn@users.noreply.github.com>
Date: Wed, 20 May 2026 13:39:02 +0200
Subject: [PATCH] feat: enhance OAuth scopes and UI for agent-driven approval
 process (#544)

* feat: enhance OAuth scopes and UI for agent-driven approval process

* refactor: update OAuth scopes to enforce explicit user consent for write and approval actions
---
 .../authorize/__tests__/route.test.ts         | 32 +++++++++++--------
 app/api/mcp-oauth/authorize/route.ts          | 19 ++++++++---
 .../mcp-oauth/token/__tests__/route.test.ts   | 27 ++++++++++++----
 extensions/general/mcp-server/server.ts       |  2 +-
 lib/auth/api-keys.ts                          | 25 +++++++++++----
 5 files changed, 73 insertions(+), 32 deletions(-)

diff --git a/app/api/mcp-oauth/authorize/__tests__/route.test.ts b/app/api/mcp-oauth/authorize/__tests__/route.test.ts
index c008c414..ace03945 100644
--- a/app/api/mcp-oauth/authorize/__tests__/route.test.ts
+++ b/app/api/mcp-oauth/authorize/__tests__/route.test.ts
@@ -103,10 +103,10 @@ describe('GET /api/mcp-oauth/authorize — CSP', () => {
 
   it('renders both read and write rows when client passes only the legacy `mcp` scope marker', async () => {
     // Claude's connector sends scope=mcp today. The consent UI must render
-    // every scope group so the user can opt into :write grants — only the
-    // :read rows are pre-checked (Art. 25(2) data-protection-by-default).
-    // Regression: commit 04c097c2 hid all write rows by clamping the
-    // ceiling to DEFAULT_OAUTH_SCOPES.
+    // every scope group so the user can opt into write/approval rows if they
+    // want — but each write/approve row MUST start unchecked. Affirmative
+    // opt-in is the access-control gate (GDPR Art. 25(2), ISO 27001:2022
+    // A.5.18 / A.8.2, SOC 2 CC6.3, ASVS V10.2.2 / V2.3.1).
     const request = new Request(
       buildAuthorizeUrl({
         response_type: 'code',
@@ -120,24 +120,28 @@ describe('GET /api/mcp-oauth/authorize — CSP', () => {
     expect(response.status).toBe(200)
     const html = await response.text()
 
-    // Write scopes must render as checkboxes (un-pre-checked).
+    // Every scope row is rendered so the user can opt into / out of each one.
     expect(html).toMatch(/value="transactions:write"/)
     expect(html).toMatch(/value="bookkeeping:write"/)
     expect(html).toMatch(/value="invoices:write"/)
     expect(html).toMatch(/value="pending_operations:approve"/)
 
-    // The :write checkbox must NOT be pre-checked when the client passed
-    // no explicit scope request — the user has to opt in deliberately.
-    const writeRow = html.match(
-      /<input[^>]*value="transactions:write"[^>]*>/
-    )?.[0]
+    // Write and approval scopes MUST render unchecked. Users have to make an
+    // affirmative, deliberate selection for each destructive permission.
+    const writeRow = html.match(/<input[^>]*value="transactions:write"[^>]*>/)?.[0]
     expect(writeRow).toBeDefined()
     expect(writeRow!).not.toContain('checked')
 
-    // The :read counterpart must still be pre-checked (safe default).
-    const readRow = html.match(
-      /<input[^>]*value="transactions:read"[^>]*>/
-    )?.[0]
+    const approveRow = html.match(/<input[^>]*value="pending_operations:approve"[^>]*>/)?.[0]
+    expect(approveRow).toBeDefined()
+    expect(approveRow!).not.toContain('checked')
+
+    const bookkeepingRow = html.match(/<input[^>]*value="bookkeeping:write"[^>]*>/)?.[0]
+    expect(bookkeepingRow).toBeDefined()
+    expect(bookkeepingRow!).not.toContain('checked')
+
+    // The :read counterpart is pre-checked (safe default).
+    const readRow = html.match(/<input[^>]*value="transactions:read"[^>]*>/)?.[0]
     expect(readRow).toBeDefined()
     expect(readRow!).toContain('checked')
   })
diff --git a/app/api/mcp-oauth/authorize/route.ts b/app/api/mcp-oauth/authorize/route.ts
index 21a8a0f1..e948e904 100644
--- a/app/api/mcp-oauth/authorize/route.ts
+++ b/app/api/mcp-oauth/authorize/route.ts
@@ -267,9 +267,20 @@ export async function GET(request: Request) {
       border: 1px solid var(--border);
       border-radius: 12px;
       padding: 2.5rem;
-      max-width: 560px;
+      max-width: 960px;
       width: 100%;
     }
+    .scope-groups {
+      display: grid;
+      grid-template-columns: repeat(2, minmax(0, 1fr));
+      column-gap: 1.5rem;
+      row-gap: 0;
+      align-items: start;
+    }
+    @media (max-width: 720px) {
+      .card { padding: 1.5rem; }
+      .scope-groups { grid-template-columns: 1fr; column-gap: 0; }
+    }
     .eyebrow {
       display: inline-flex;
       align-items: center;
@@ -369,10 +380,8 @@ export async function GET(request: Request) {
       outline-offset: 2px;
     }
     .scope-group {
-      padding: 0.375rem 0;
-      border-bottom: 1px solid var(--border);
+      padding: 0.375rem 0 0.75rem;
     }
-    .scope-group:last-of-type { border-bottom: none; padding-bottom: 0; }
     .scope-group-title {
       font-size: 0.6875rem;
       font-weight: 500;
@@ -530,7 +539,7 @@ export async function GET(request: Request) {
         </div>
       </div>
 
-      ${scopeCheckboxesHtml}
+      <div class="scope-groups">${scopeCheckboxesHtml}</div>
 
       <div class="warn">
         <svg class="warn-icon" viewBox="0 0 16 16" fill="none" stroke="currentColor" stroke-width="1.5" aria-hidden="true">
diff --git a/app/api/mcp-oauth/token/__tests__/route.test.ts b/app/api/mcp-oauth/token/__tests__/route.test.ts
index d3a16d27..f312ec6c 100644
--- a/app/api/mcp-oauth/token/__tests__/route.test.ts
+++ b/app/api/mcp-oauth/token/__tests__/route.test.ts
@@ -296,14 +296,25 @@ describe('POST /api/mcp-oauth/token', () => {
       )
       expect(res.status).toBe(200)
       const body = await res.json()
-      // DEFAULT_OAUTH_SCOPES is read-only by design (GDPR Art. 25(2) —
-      // destructive scopes must be requested explicitly).
+      // DEFAULT_OAUTH_SCOPES is read-only by design. Write and approval scopes
+      // must be requested explicitly by the client AND ticked by the user on
+      // the consent screen — GDPR Art. 25(2), ISO 27001:2022 A.5.18 / A.8.2,
+      // SOC 2 CC6.3, ASVS V8.1.1 / V10.2.1.
       const granted = body.scope.split(' ')
       expect(granted).toContain('transactions:read')
+      expect(granted).toContain('invoices:read')
+      expect(granted).toContain('suppliers:read')
       expect(granted).toContain('reports:read')
-      expect(granted).not.toContain('bookkeeping:write')
-      expect(granted).not.toContain('pending_operations:approve')
+      // No silent write or approval grants:
       expect(granted).not.toContain('transactions:write')
+      expect(granted).not.toContain('invoices:write')
+      expect(granted).not.toContain('suppliers:write')
+      expect(granted).not.toContain('customers:write')
+      expect(granted).not.toContain('documents:write')
+      expect(granted).not.toContain('pending_operations:approve')
+      expect(granted).not.toContain('bookkeeping:write')
+      expect(granted).not.toContain('payroll:write')
+      expect(granted).not.toContain('webhooks:manage')
     })
 
     it('honours scopes from the auth code when present', async () => {
@@ -403,7 +414,7 @@ describe('POST /api/mcp-oauth/token', () => {
       )
     })
 
-    it('falls back to DEFAULT_OAUTH_SCOPES for legacy keys with null scopes', async () => {
+    it('falls back to read-only DEFAULT_OAUTH_SCOPES for legacy keys with null scopes', async () => {
       const { supabase, enqueueMany } = createQueuedMockSupabase()
       mocks.supabaseFactory.mockReturnValue(supabase)
       enqueueMany([
@@ -421,8 +432,12 @@ describe('POST /api/mcp-oauth/token', () => {
       const body = await res.json()
       const granted = body.scope.split(' ')
       expect(granted).toContain('transactions:read')
-      expect(granted).not.toContain('bookkeeping:write')
+      // No silent grant of write or approval scopes (GDPR Art. 25(2),
+      // SoD per findStageApproveConflict — see lib/auth/api-keys.ts).
+      expect(granted).not.toContain('transactions:write')
       expect(granted).not.toContain('pending_operations:approve')
+      expect(granted).not.toContain('bookkeeping:write')
+      expect(granted).not.toContain('payroll:write')
     })
   })
 })
diff --git a/extensions/general/mcp-server/server.ts b/extensions/general/mcp-server/server.ts
index 8ad078dc..a55e84b8 100644
--- a/extensions/general/mcp-server/server.ts
+++ b/extensions/general/mcp-server/server.ts
@@ -6258,7 +6258,7 @@ export const tools: McpTool[] = [
 
   {
     name: 'gnubok_approve_pending_operation',
-    description: 'Approve a staged pending_operation. Runs the same commit path as web-UI approval — atomic claim → executor → status update. High-risk operations require confirmed=true. Returns status=committed on success, or status=rejected/failed with error details.',
+    description: 'Commit a pending_operation. Caller must hold the pending_operations:approve scope and pass confirmed=true for risk_level=high ops (BFL 5 kap 5§ irreversible postings). Call only after the user has affirmatively confirmed the specific operation_id.',
     inputSchema: {
       type: 'object',
       additionalProperties: false,
diff --git a/lib/auth/api-keys.ts b/lib/auth/api-keys.ts
index 32eaa71d..53b42bc6 100644
--- a/lib/auth/api-keys.ts
+++ b/lib/auth/api-keys.ts
@@ -45,12 +45,25 @@ export const DEFAULT_SCOPES: ApiKeyScope[] = [
 ]
 
 /**
- * Read-only fallback granted to OAuth-issued keys when the client did not
- * pass an explicit `scope` parameter at /authorize. Per GDPR Art. 25(2)
- * (data protection by default), the silent fallback must never include
- * destructive scopes (*:write, pending_operations:approve, bookkeeping:write).
- * Destructive scopes must be requested explicitly by the client and consented
- * to by the user.
+ * Default scope grant for OAuth-issued keys when the client did not pass an
+ * explicit `scope` parameter at /authorize. Read-only by design — every
+ * write or approval scope must be requested explicitly by the client AND
+ * affirmatively ticked by the user on the consent screen.
+ *
+ * Rationale (do not weaken without a documented security decision):
+ *   - GDPR Art. 25(2) data-protection-by-default: the minimum-necessary
+ *     access set must be the silent baseline.
+ *   - ISO 27001:2022 A.5.18 / A.8.2 / SOC 2 CC6.3: privileged capabilities
+ *     (write, approve) must not be bundled into a default grant.
+ *   - Segregation of Duties (findStageApproveConflict below): granting any
+ *     STAGING_SCOPES member together with `pending_operations:approve` on a
+ *     single key lets an automated agent both stage AND commit financial
+ *     postings without a human-in-the-loop review. Keeping the default
+ *     read-only prevents this combination from being silently issued.
+ *   - BFL 5 kap 5§ / BFNAR 2013:2 behandlingshistorik: write paths that
+ *     create or modify verifikationer must be opt-in at the authorization
+ *     layer; conversational acknowledgement at the agent layer is not an
+ *     auditable substitute.
  */
 export const DEFAULT_OAUTH_SCOPES: ApiKeyScope[] = [
   'transactions:read',