refactor(mcp): address review feedback
- Keep PR1 strictly read-only: drop the scope selector, the read_write request rule and mcp:write granting from the UI/controller (the write scope returns with the write tools in PR2). Removes a token that could be minted over-privileged with a false "can edit data" UI promise. - Gate via User::canUseFeature(PlanFeature::McpAccess) instead of hasProPlan() directly, matching the existing feature-gating convention. - Confirm token rotation behind an AlertDialog (it breaks connected clients), matching revoke; drop the silent on-load clipboard auto-copy. - Give the data-egress disclaimer a destructive variant for visual weight. - Extract the duplicated token-ownership check; document the controller-reuse coupling and the shared-tenant space-scoping model; fix ListSpaces N+1 and a duplicate output key.
This commit is contained in:
Víctor Falcón
parent
14ab73dcfb
commit
fcf5e6e46f
|
|
@ -28,15 +28,12 @@ class McpTokenController extends Controller
|
|||
|
||||
/**
|
||||
* Create a new MCP token. The plaintext secret is flashed once; only its
|
||||
* hash is stored, so it can never be shown again.
|
||||
* hash is stored, so it can never be shown again. Tokens are read-only for
|
||||
* now (the `mcp:write` ability is introduced alongside the write tools).
|
||||
*/
|
||||
public function store(StoreMcpTokenRequest $request): RedirectResponse
|
||||
{
|
||||
$abilities = $request->validated('scope') === 'read_write'
|
||||
? ['mcp:read', 'mcp:write']
|
||||
: ['mcp:read'];
|
||||
|
||||
$token = $request->user()->createToken($request->validated('name'), $abilities);
|
||||
$token = $request->user()->createToken($request->validated('name'), ['mcp:read']);
|
||||
|
||||
return to_route('mcp.index')->with('mcp_token', $token->plainTextToken);
|
||||
}
|
||||
|
|
@ -46,11 +43,7 @@ class McpTokenController extends Controller
|
|||
*/
|
||||
public function destroy(Request $request, PersonalAccessToken $token): RedirectResponse
|
||||
{
|
||||
abort_unless(
|
||||
$token->tokenable_id === $request->user()->getKey()
|
||||
&& $token->tokenable_type === $request->user()->getMorphClass(),
|
||||
403
|
||||
);
|
||||
$this->authorizeOwnership($request, $token);
|
||||
|
||||
$token->delete();
|
||||
|
||||
|
|
@ -63,11 +56,7 @@ class McpTokenController extends Controller
|
|||
*/
|
||||
public function rotate(Request $request, PersonalAccessToken $token): RedirectResponse
|
||||
{
|
||||
abort_unless(
|
||||
$token->tokenable_id === $request->user()->getKey()
|
||||
&& $token->tokenable_type === $request->user()->getMorphClass(),
|
||||
403
|
||||
);
|
||||
$this->authorizeOwnership($request, $token);
|
||||
|
||||
$fresh = $request->user()->createToken($token->name, $token->abilities);
|
||||
$token->delete();
|
||||
|
|
@ -75,6 +64,18 @@ class McpTokenController extends Controller
|
|||
return to_route('mcp.index')->with('mcp_token', $fresh->plainTextToken);
|
||||
}
|
||||
|
||||
/**
|
||||
* Ensure the token belongs to the requesting user before mutating it.
|
||||
*/
|
||||
private function authorizeOwnership(Request $request, PersonalAccessToken $token): void
|
||||
{
|
||||
abort_unless(
|
||||
$token->tokenable_id === $request->user()->getKey()
|
||||
&& $token->tokenable_type === $request->user()->getMorphClass(),
|
||||
403
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* @return list<array{id: int|string, name: string, scope: string, created_at: ?string, last_used_at: ?string}>
|
||||
*/
|
||||
|
|
|
|||
|
|
@ -24,7 +24,6 @@ class StoreMcpTokenRequest extends FormRequest
|
|||
{
|
||||
return [
|
||||
'name' => ['required', 'string', 'max:255'],
|
||||
'scope' => ['required', 'in:read,read_write'],
|
||||
];
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -29,9 +29,7 @@ class ListSpaces extends McpTool
|
|||
'id' => $space->id,
|
||||
'name' => $space->name,
|
||||
'personal' => $space->personal,
|
||||
'is_default' => $space->personal,
|
||||
'is_current' => $space->id === $user->current_space_id,
|
||||
'role' => $space->roleFor($user),
|
||||
]);
|
||||
|
||||
return $this->json(['spaces' => $spaces]);
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@
|
|||
|
||||
namespace App\Mcp\Tools;
|
||||
|
||||
use App\Enums\PlanFeature;
|
||||
use App\Models\Space;
|
||||
use App\Models\User;
|
||||
use Illuminate\Support\Str;
|
||||
|
|
@ -34,7 +35,7 @@ abstract class McpTool extends Tool
|
|||
return Response::error('Authentication required.');
|
||||
}
|
||||
|
||||
if (! $user->hasProPlan()) {
|
||||
if (! $user->canUseFeature(PlanFeature::McpAccess)) {
|
||||
return Response::error(
|
||||
'A paid (Pro) plan is required to use the Whisper Money MCP. Upgrade your account at '.route('subscribe')
|
||||
);
|
||||
|
|
@ -58,8 +59,12 @@ abstract class McpTool extends Tool
|
|||
* a synthesized GET request bound to the MCP user, returning its JSON body.
|
||||
* Keeps the (user-scoped) dashboard maths in exactly one place.
|
||||
*
|
||||
* ponytail: couples to the controllers returning a JsonResponse; acceptable
|
||||
* while they're stable. Extract the orchestration into a shared service if a
|
||||
* controller stops returning JSON or a third tool needs the same maths.
|
||||
*
|
||||
* @param array<string, mixed> $query
|
||||
* @return array<string, mixed>
|
||||
* @return array<array-key, mixed>
|
||||
*/
|
||||
protected function callController(object $controller, string $method, User $user, array $query): array
|
||||
{
|
||||
|
|
@ -72,6 +77,11 @@ abstract class McpTool extends Tool
|
|||
/**
|
||||
* The space a tool operates on: the optional `space` argument (validated
|
||||
* against the spaces the user can access) or the user's personal space.
|
||||
*
|
||||
* Scoping is by `space_id` only, gated by membership (`accessibleSpaces`): a
|
||||
* space is a shared tenant, so a member is meant to see every row in it. The
|
||||
* security boundary is the membership check here, not a per-row `user_id`
|
||||
* filter.
|
||||
*/
|
||||
protected function resolveSpace(Request $request, User $user): Space
|
||||
{
|
||||
|
|
|
|||
|
|
@ -2255,5 +2255,8 @@
|
|||
"Claude Code": "Claude Code",
|
||||
"ChatGPT": "ChatGPT",
|
||||
"Enable developer mode, then Settings → Connectors → Create. Paste the URL above and add an \"Authorization: Bearer <token>\" header.": "Activa el modo desarrollador, luego Ajustes → Conectores → Crear. Pega la URL de arriba y añade una cabecera «Authorization: Bearer <token>».",
|
||||
"This is the only time it is shown. Store it somewhere safe — you won't be able to see it again.": "Es la única vez que se muestra. Guárdalo en un lugar seguro: no podrás verlo de nuevo."
|
||||
"This is the only time it is shown. Store it somewhere safe — you won't be able to see it again.": "Es la única vez que se muestra. Guárdalo en un lugar seguro: no podrás verlo de nuevo.",
|
||||
"Tokens are read-only: they can analyse your data but never change it.": "Los tokens son de solo lectura: pueden analizar tus datos pero nunca modificarlos.",
|
||||
"Rotate this token?": "¿Rotar este token?",
|
||||
"Rotating replaces the secret. Any AI client using the current token will stop working until you reconnect it with the new one.": "Rotar reemplaza el secreto. Cualquier cliente de IA que use el token actual dejará de funcionar hasta que lo reconectes con el nuevo."
|
||||
}
|
||||
|
|
|
|||
|
|
@ -28,13 +28,6 @@ import {
|
|||
} from '@/components/ui/card';
|
||||
import { Input } from '@/components/ui/input';
|
||||
import { Label } from '@/components/ui/label';
|
||||
import {
|
||||
Select,
|
||||
SelectContent,
|
||||
SelectItem,
|
||||
SelectTrigger,
|
||||
SelectValue,
|
||||
} from '@/components/ui/select';
|
||||
import { useClipboard } from '@/hooks/use-clipboard';
|
||||
import AppLayout from '@/layouts/app-layout';
|
||||
import SettingsLayout from '@/layouts/settings/layout';
|
||||
|
|
@ -42,7 +35,6 @@ import { type BreadcrumbItem, type SharedData } from '@/types';
|
|||
import { __ } from '@/utils/i18n';
|
||||
import { Head, Link, router, useForm, usePage } from '@inertiajs/react';
|
||||
import { Copy, KeyRound, RefreshCw, Trash2 } from 'lucide-react';
|
||||
import { useEffect } from 'react';
|
||||
import { toast } from 'sonner';
|
||||
|
||||
interface TokenRow {
|
||||
|
|
@ -73,19 +65,7 @@ export default function Mcp() {
|
|||
usePage<SharedData & McpPageProps>().props;
|
||||
const [, copy] = useClipboard();
|
||||
|
||||
const form = useForm({ name: '', scope: 'read' });
|
||||
|
||||
useEffect(() => {
|
||||
if (newToken) {
|
||||
copy(newToken).then((ok) => {
|
||||
if (ok) {
|
||||
toast.success(__('Token copied to clipboard'));
|
||||
}
|
||||
});
|
||||
}
|
||||
// Only react to a freshly created token.
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
}, [newToken]);
|
||||
const form = useForm({ name: '' });
|
||||
|
||||
function createToken(event: React.FormEvent) {
|
||||
event.preventDefault();
|
||||
|
|
@ -138,7 +118,7 @@ export default function Mcp() {
|
|||
</Alert>
|
||||
)}
|
||||
|
||||
<Alert>
|
||||
<Alert variant="destructive">
|
||||
<AlertTitle>{__('Your data leaves Whisper Money')}</AlertTitle>
|
||||
<AlertDescription>
|
||||
{__(
|
||||
|
|
@ -181,7 +161,7 @@ export default function Mcp() {
|
|||
<CardTitle>{__('Create a token')}</CardTitle>
|
||||
<CardDescription>
|
||||
{__(
|
||||
'Read-only tokens can only analyse data. Read & write tokens can also create and edit data.',
|
||||
'Tokens are read-only: they can analyse your data but never change it.',
|
||||
)}
|
||||
</CardDescription>
|
||||
</CardHeader>
|
||||
|
|
@ -204,32 +184,6 @@ export default function Mcp() {
|
|||
required
|
||||
/>
|
||||
</div>
|
||||
<div className="space-y-2">
|
||||
<Label htmlFor="token-scope">
|
||||
{__('Scope')}
|
||||
</Label>
|
||||
<Select
|
||||
value={form.data.scope}
|
||||
onValueChange={(value) =>
|
||||
form.setData('scope', value)
|
||||
}
|
||||
>
|
||||
<SelectTrigger
|
||||
id="token-scope"
|
||||
className="w-full sm:w-56"
|
||||
>
|
||||
<SelectValue />
|
||||
</SelectTrigger>
|
||||
<SelectContent>
|
||||
<SelectItem value="read">
|
||||
{__('Read only')}
|
||||
</SelectItem>
|
||||
<SelectItem value="read_write">
|
||||
{__('Read & write')}
|
||||
</SelectItem>
|
||||
</SelectContent>
|
||||
</Select>
|
||||
</div>
|
||||
<Button
|
||||
type="submit"
|
||||
disabled={form.processing}
|
||||
|
|
@ -291,23 +245,52 @@ export default function Mcp() {
|
|||
</p>
|
||||
</div>
|
||||
<div className="flex items-center gap-2">
|
||||
<Button
|
||||
variant="outline"
|
||||
size="sm"
|
||||
onClick={() =>
|
||||
router.post(
|
||||
rotate(token.id).url,
|
||||
{},
|
||||
{
|
||||
preserveScroll:
|
||||
true,
|
||||
},
|
||||
)
|
||||
}
|
||||
>
|
||||
<RefreshCw className="h-4 w-4" />
|
||||
{__('Rotate')}
|
||||
</Button>
|
||||
<AlertDialog>
|
||||
<AlertDialogTrigger asChild>
|
||||
<Button
|
||||
variant="outline"
|
||||
size="sm"
|
||||
>
|
||||
<RefreshCw className="h-4 w-4" />
|
||||
{__('Rotate')}
|
||||
</Button>
|
||||
</AlertDialogTrigger>
|
||||
<AlertDialogContent>
|
||||
<AlertDialogHeader>
|
||||
<AlertDialogTitle>
|
||||
{__(
|
||||
'Rotate this token?',
|
||||
)}
|
||||
</AlertDialogTitle>
|
||||
<AlertDialogDescription>
|
||||
{__(
|
||||
'Rotating replaces the secret. Any AI client using the current token will stop working until you reconnect it with the new one.',
|
||||
)}
|
||||
</AlertDialogDescription>
|
||||
</AlertDialogHeader>
|
||||
<AlertDialogFooter>
|
||||
<AlertDialogCancel>
|
||||
{__('Cancel')}
|
||||
</AlertDialogCancel>
|
||||
<AlertDialogAction
|
||||
onClick={() =>
|
||||
router.post(
|
||||
rotate(
|
||||
token.id,
|
||||
).url,
|
||||
{},
|
||||
{
|
||||
preserveScroll:
|
||||
true,
|
||||
},
|
||||
)
|
||||
}
|
||||
>
|
||||
{__('Rotate')}
|
||||
</AlertDialogAction>
|
||||
</AlertDialogFooter>
|
||||
</AlertDialogContent>
|
||||
</AlertDialog>
|
||||
<AlertDialog>
|
||||
<AlertDialogTrigger asChild>
|
||||
<Button
|
||||
|
|
|
|||
|
|
@ -15,11 +15,11 @@ it('renders the MCP settings page', function () {
|
|||
->assertOk();
|
||||
});
|
||||
|
||||
it('creates a read-only token by default and flashes the secret once', function () {
|
||||
it('creates a read-only token and flashes the secret once', function () {
|
||||
$user = User::factory()->create();
|
||||
|
||||
actingAs($user)
|
||||
->post(route('mcp.tokens.store'), ['name' => 'Claude Desktop', 'scope' => 'read'])
|
||||
->post(route('mcp.tokens.store'), ['name' => 'Claude Desktop'])
|
||||
->assertRedirect(route('mcp.index'))
|
||||
->assertSessionHas('mcp_token');
|
||||
|
||||
|
|
@ -29,18 +29,10 @@ it('creates a read-only token by default and flashes the secret once', function
|
|||
expect($token->abilities)->toBe(['mcp:read']);
|
||||
});
|
||||
|
||||
it('creates a read-write token when requested', function () {
|
||||
$user = User::factory()->create();
|
||||
|
||||
actingAs($user)->post(route('mcp.tokens.store'), ['name' => 'CC', 'scope' => 'read_write']);
|
||||
|
||||
expect($user->tokens()->first()->abilities)->toBe(['mcp:read', 'mcp:write']);
|
||||
});
|
||||
|
||||
it('validates the token scope', function () {
|
||||
it('requires a token name', function () {
|
||||
actingAs(User::factory()->create())
|
||||
->post(route('mcp.tokens.store'), ['name' => 'X', 'scope' => 'admin'])
|
||||
->assertSessionHasErrors('scope');
|
||||
->post(route('mcp.tokens.store'), ['name' => ''])
|
||||
->assertSessionHasErrors('name');
|
||||
});
|
||||
|
||||
it('lets a free account create a token (gating happens at request time)', function () {
|
||||
|
|
@ -48,7 +40,7 @@ it('lets a free account create a token (gating happens at request time)', functi
|
|||
$user = User::factory()->create();
|
||||
|
||||
actingAs($user)
|
||||
->post(route('mcp.tokens.store'), ['name' => 'Free', 'scope' => 'read'])
|
||||
->post(route('mcp.tokens.store'), ['name' => 'Free'])
|
||||
->assertSessionHas('mcp_token');
|
||||
|
||||
expect($user->tokens()->count())->toBe(1);
|
||||
|
|
@ -79,7 +71,7 @@ it('cannot revoke another user\'s token', function () {
|
|||
|
||||
it('rotates a token, replacing the secret but keeping the scope', function () {
|
||||
$user = User::factory()->create();
|
||||
$token = $user->createToken('X', ['mcp:read', 'mcp:write'])->accessToken;
|
||||
$token = $user->createToken('X', ['mcp:read'])->accessToken;
|
||||
|
||||
actingAs($user)
|
||||
->post(route('mcp.tokens.rotate', $token->id))
|
||||
|
|
@ -89,5 +81,5 @@ it('rotates a token, replacing the secret but keeping the scope', function () {
|
|||
|
||||
expect($tokens)->toHaveCount(1);
|
||||
expect($tokens->first()->id)->not->toBe($token->id);
|
||||
expect($tokens->first()->abilities)->toBe(['mcp:read', 'mcp:write']);
|
||||
expect($tokens->first()->abilities)->toBe(['mcp:read']);
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in New Issue