diff --git a/completions/mcpctl.bash b/completions/mcpctl.bash index 79db36d..39ec089 100644 --- a/completions/mcpctl.bash +++ b/completions/mcpctl.bash @@ -194,7 +194,7 @@ _mcpctl() { COMPREPLY=($(compgen -W "--description --member --force -h --help" -- "$cur")) ;; rbac) - COMPREPLY=($(compgen -W "--subject --binding --operation --force -h --help" -- "$cur")) + COMPREPLY=($(compgen -W "--subject --roleBindings --force -h --help" -- "$cur")) ;; prompt) COMPREPLY=($(compgen -W "-p --project --content --content-file --priority --link -h --help" -- "$cur")) diff --git a/completions/mcpctl.fish b/completions/mcpctl.fish index 3f3c869..736eb78 100644 --- a/completions/mcpctl.fish +++ b/completions/mcpctl.fish @@ -332,8 +332,7 @@ complete -c mcpctl -n "__mcpctl_subcmd_active create group" -l force -d 'Update # create rbac options complete -c mcpctl -n "__mcpctl_subcmd_active create rbac" -l subject -d 'Subject as Kind:name (repeat for multiple)' -x -complete -c mcpctl -n "__mcpctl_subcmd_active create rbac" -l binding -d 'Role binding as role:resource (e.g. edit:servers, run:projects)' -x -complete -c mcpctl -n "__mcpctl_subcmd_active create rbac" -l operation -d 'Operation binding (e.g. logs, backup)' -x +complete -c mcpctl -n "__mcpctl_subcmd_active create rbac" -l roleBindings -d 'Role binding as key:value pairs, e.g. "role:view,resource:servers" or "role:view,resource:servers,name:my-ha" or "action:logs" (repeat for multiple)' -x complete -c mcpctl -n "__mcpctl_subcmd_active create rbac" -l force -d 'Update if already exists' # create prompt options diff --git a/docs/mcptoken-implementation.md b/docs/mcptoken-implementation.md index 93fdafb..85b5cdb 100644 --- a/docs/mcptoken-implementation.md +++ b/docs/mcptoken-implementation.md @@ -48,9 +48,39 @@ Before starting the feature, we flushed your in-flight changes to main so they w - The mcpd **auth middleware** does not yet dispatch on the token prefix. A raw `mcpctl_pat_…` bearer sent to any `/api/v1/*` endpoint (other than `/introspect`) is still rejected as an invalid session. That's intentional — PR 3 extends `middleware/auth.ts` to recognize both session bearers and McpToken bearers. - No CLI yet. Tokens can be created only via `POST /api/v1/mcptokens` for now. -## PR 2 — RBAC CLI migration +## PR 2 — RBAC CLI migration ✅ -_(blocked by PR 1 — parser is reused by PR 3)_ +Migrated `mcpctl create rbac` from positional flag syntax to the key=value form you asked for. + +Before: +``` +mcpctl create rbac developers \ + --subject User:alice@test.com \ + --binding edit:servers \ + --binding view:servers:my-ha \ + --operation logs +``` +After: +``` +mcpctl create rbac developers \ + --subject User:alice@test.com \ + --roleBindings role:edit,resource:servers \ + --roleBindings role:view,resource:servers,name:my-ha \ + --roleBindings action:logs +``` + +| # | Step | Status | +|---|---|---| +| 1 | New shared parser at `src/cli/src/commands/rbac-bindings.ts` exporting `parseRoleBinding(entry)` | ✅ | +| 2 | `src/cli/src/commands/create.ts` — old `--binding`/`--operation` flags replaced with one repeatable `--roleBindings `. Uses the new parser. | ✅ | +| 3 | Tests in `src/cli/tests/commands/create.test.ts` rewritten to the new form (8 RBAC tests updated) | ✅ | +| 4 | New dedicated unit test `src/cli/tests/commands/rbac-bindings.test.ts` — 9 cases covering unscoped / name-scoped / action / trim / empty-value / unknown-key / action-conflict / missing-role rejections | ✅ | +| 5 | Shell completions regenerated via `pnpm completions:generate` — both `completions/mcpctl.{bash,fish}` now offer `--roleBindings`, no longer `--binding`/`--operation` | ✅ | +| 6 | Nothing in `docs/` or `README.md` referenced the old flags | ✅ | + +Full CLI suite still 406/406 green. On-disk YAML shape (`roleBindings: [...]`) is unchanged, so backups and existing `apply -f` files keep working. + +The extracted `parseRoleBinding` helper is what PR 3's `mcpctl create mcptoken --bind ` flag will reuse. ## PR 3 — CLI mcptoken verbs + mcpd auth dispatch + audit diff --git a/src/cli/src/commands/create.ts b/src/cli/src/commands/create.ts index 7954884..b863181 100644 --- a/src/cli/src/commands/create.ts +++ b/src/cli/src/commands/create.ts @@ -1,6 +1,7 @@ import { Command } from 'commander'; import { type ApiClient, ApiError } from '../api-client.js'; import { resolveNameOrId } from './shared.js'; +import { parseRoleBinding } from './rbac-bindings.js'; export interface CreateCommandDeps { client: ApiClient; log: (...args: unknown[]) => void; @@ -331,8 +332,12 @@ export function createCreateCommand(deps: CreateCommandDeps): Command { .description('Create an RBAC binding definition') .argument('', 'RBAC binding name') .option('--subject ', 'Subject as Kind:name (repeat for multiple)', collect, []) - .option('--binding ', 'Role binding as role:resource (e.g. edit:servers, run:projects)', collect, []) - .option('--operation ', 'Operation binding (e.g. logs, backup)', collect, []) + .option( + '--roleBindings ', + 'Role binding as key:value pairs, e.g. "role:view,resource:servers" or "role:view,resource:servers,name:my-ha" or "action:logs" (repeat for multiple)', + collect, + [], + ) .option('--force', 'Update if already exists') .action(async (name: string, opts) => { const subjects = (opts.subject as string[]).map((entry: string) => { @@ -343,24 +348,7 @@ export function createCreateCommand(deps: CreateCommandDeps): Command { return { kind: entry.slice(0, colonIdx), name: entry.slice(colonIdx + 1) }; }); - const roleBindings: Array> = []; - - // Resource bindings from --binding flag (role:resource or role:resource:name) - for (const entry of opts.binding as string[]) { - const parts = entry.split(':'); - if (parts.length === 2) { - roleBindings.push({ role: parts[0]!, resource: parts[1]! }); - } else if (parts.length === 3) { - roleBindings.push({ role: parts[0]!, resource: parts[1]!, name: parts[2]! }); - } else { - throw new Error(`Invalid binding format '${entry}'. Expected role:resource or role:resource:name (e.g. edit:servers, view:servers:my-ha)`); - } - } - - // Operation bindings from --operation flag - for (const action of opts.operation as string[]) { - roleBindings.push({ role: 'run', action }); - } + const roleBindings = (opts.roleBindings as string[]).map((entry: string) => parseRoleBinding(entry)); const body: Record = { name, diff --git a/src/cli/src/commands/rbac-bindings.ts b/src/cli/src/commands/rbac-bindings.ts new file mode 100644 index 0000000..9601ee8 --- /dev/null +++ b/src/cli/src/commands/rbac-bindings.ts @@ -0,0 +1,49 @@ +/** + * Parse one `--roleBindings ` entry into a role-binding object the API accepts. + * + * Accepted forms: + * role:view,resource:servers → resource binding (unscoped) + * role:view,resource:servers,name:my-ha → resource binding (name-scoped) + * action:logs → operation binding (role:run is implied) + * + * Whitespace around keys/values is trimmed. Keys must be one of: role, resource, name, action. + */ +export type RoleBindingEntry = + | { role: string; resource: string; name?: string } + | { role: 'run'; action: string }; + +export function parseRoleBinding(entry: string): RoleBindingEntry { + const pairs: Record = {}; + for (const part of entry.split(',')) { + const colonIdx = part.indexOf(':'); + if (colonIdx === -1) { + throw new Error(`Invalid roleBindings entry '${entry}': expected key:value pairs separated by commas`); + } + const key = part.slice(0, colonIdx).trim(); + const value = part.slice(colonIdx + 1).trim(); + if (!key || !value) { + throw new Error(`Invalid roleBindings entry '${entry}': empty key or value`); + } + if (!['role', 'resource', 'name', 'action'].includes(key)) { + throw new Error(`Invalid roleBindings key '${key}' in '${entry}': expected one of role, resource, name, action`); + } + pairs[key] = value; + } + + // Operation binding: presence of `action:` implies role:run + if (pairs['action'] !== undefined) { + if (pairs['resource'] !== undefined || pairs['name'] !== undefined) { + throw new Error(`Invalid roleBindings entry '${entry}': 'action' cannot be combined with 'resource' or 'name'`); + } + return { role: 'run', action: pairs['action'] }; + } + + // Resource binding + if (pairs['role'] === undefined || pairs['resource'] === undefined) { + throw new Error(`Invalid roleBindings entry '${entry}': need either 'action:…' or both 'role:…,resource:…'`); + } + if (pairs['name'] !== undefined) { + return { role: pairs['role'], resource: pairs['resource'], name: pairs['name'] }; + } + return { role: pairs['role'], resource: pairs['resource'] }; +} diff --git a/src/cli/tests/commands/create.test.ts b/src/cli/tests/commands/create.test.ts index fe8eb0f..d1d2f32 100644 --- a/src/cli/tests/commands/create.test.ts +++ b/src/cli/tests/commands/create.test.ts @@ -318,8 +318,8 @@ describe('create command', () => { 'rbac', 'developers', '--subject', 'User:alice@test.com', '--subject', 'Group:dev-team', - '--binding', 'edit:servers', - '--binding', 'view:instances', + '--roleBindings', 'role:edit,resource:servers', + '--roleBindings', 'role:view,resource:instances', ], { from: 'user' }); expect(client.post).toHaveBeenCalledWith('/api/v1/rbac', { @@ -342,7 +342,7 @@ describe('create command', () => { await cmd.parseAsync([ 'rbac', 'admins', '--subject', 'User:admin@test.com', - '--binding', 'edit:*', + '--roleBindings', 'role:edit,resource:*', ], { from: 'user' }); expect(client.post).toHaveBeenCalledWith('/api/v1/rbac', { @@ -371,18 +371,18 @@ describe('create command', () => { ).rejects.toThrow('Invalid subject format'); }); - it('throws on invalid binding format', async () => { + it('throws on invalid roleBindings format', async () => { const cmd = createCreateCommand({ client, log }); await expect( - cmd.parseAsync(['rbac', 'bad', '--binding', 'no-colon'], { from: 'user' }), - ).rejects.toThrow('Invalid binding format'); + cmd.parseAsync(['rbac', 'bad', '--roleBindings', 'no-colon'], { from: 'user' }), + ).rejects.toThrow(/Invalid roleBindings/); }); it('throws on 409 without --force', async () => { vi.mocked(client.post).mockRejectedValueOnce(new ApiError(409, '{"error":"RBAC already exists"}')); const cmd = createCreateCommand({ client, log }); await expect( - cmd.parseAsync(['rbac', 'developers', '--subject', 'User:a@b.com', '--binding', 'edit:servers'], { from: 'user' }), + cmd.parseAsync(['rbac', 'developers', '--subject', 'User:a@b.com', '--roleBindings', 'role:edit,resource:servers'], { from: 'user' }), ).rejects.toThrow('API error 409'); }); @@ -393,7 +393,7 @@ describe('create command', () => { await cmd.parseAsync([ 'rbac', 'developers', '--subject', 'User:new@test.com', - '--binding', 'edit:*', + '--roleBindings', 'role:edit,resource:*', '--force', ], { from: 'user' }); @@ -404,15 +404,15 @@ describe('create command', () => { expect(output.join('\n')).toContain("rbac 'developers' updated"); }); - it('creates an RBAC definition with operation bindings', async () => { + it('creates an RBAC definition with operation bindings (action:… shorthand)', async () => { vi.mocked(client.post).mockResolvedValueOnce({ id: 'rbac-1', name: 'ops' }); const cmd = createCreateCommand({ client, log }); await cmd.parseAsync([ 'rbac', 'ops', '--subject', 'Group:ops-team', - '--binding', 'edit:servers', - '--operation', 'logs', - '--operation', 'backup', + '--roleBindings', 'role:edit,resource:servers', + '--roleBindings', 'action:logs', + '--roleBindings', 'action:backup', ], { from: 'user' }); expect(client.post).toHaveBeenCalledWith('/api/v1/rbac', { @@ -433,7 +433,7 @@ describe('create command', () => { await cmd.parseAsync([ 'rbac', 'ha-viewer', '--subject', 'User:alice@test.com', - '--binding', 'view:servers:my-ha', + '--roleBindings', 'role:view,resource:servers,name:my-ha', ], { from: 'user' }); expect(client.post).toHaveBeenCalledWith('/api/v1/rbac', { diff --git a/src/cli/tests/commands/rbac-bindings.test.ts b/src/cli/tests/commands/rbac-bindings.test.ts new file mode 100644 index 0000000..9a1ec6a --- /dev/null +++ b/src/cli/tests/commands/rbac-bindings.test.ts @@ -0,0 +1,54 @@ +import { describe, it, expect } from 'vitest'; +import { parseRoleBinding } from '../../src/commands/rbac-bindings.js'; + +describe('parseRoleBinding', () => { + it('parses an unscoped resource binding', () => { + expect(parseRoleBinding('role:view,resource:servers')).toEqual({ + role: 'view', + resource: 'servers', + }); + }); + + it('parses a name-scoped resource binding', () => { + expect(parseRoleBinding('role:view,resource:servers,name:my-ha')).toEqual({ + role: 'view', + resource: 'servers', + name: 'my-ha', + }); + }); + + it('parses an operation binding via the action shorthand', () => { + expect(parseRoleBinding('action:logs')).toEqual({ + role: 'run', + action: 'logs', + }); + }); + + it('trims whitespace around keys and values', () => { + expect(parseRoleBinding('role: edit , resource: * ')).toEqual({ + role: 'edit', + resource: '*', + }); + }); + + it('rejects a pair with no colon', () => { + expect(() => parseRoleBinding('role=view')).toThrow(/key:value pairs/); + }); + + it('rejects an unknown key', () => { + expect(() => parseRoleBinding('role:view,resource:servers,scope:project')).toThrow(/Invalid roleBindings key 'scope'/); + }); + + it('rejects an empty value', () => { + expect(() => parseRoleBinding('role:view,resource:')).toThrow(/empty key or value/); + }); + + it('rejects action combined with resource/name', () => { + expect(() => parseRoleBinding('action:logs,resource:servers')).toThrow(/cannot be combined/); + }); + + it('requires both role and resource when action is absent', () => { + expect(() => parseRoleBinding('role:view')).toThrow(/need either 'action/); + expect(() => parseRoleBinding('resource:servers')).toThrow(/need either 'action/); + }); +});