From 9a3dc175ef66efcb9893cf35be6a438e11d73216 Mon Sep 17 00:00:00 2001 From: Juergen Klaassen Date: Tue, 28 Apr 2026 10:09:07 -0600 Subject: [PATCH] feat(mcp): profile-level read-only enforcement (KLA-409) Closes the misconfiguration vector that --read-only alone leaves open: an operator who provisioned a JumpCloud OAuth client with read-only API scope and bound it 1:1 to a jc profile could still accidentally start 'jc mcp serve' against that profile *without* --read-only and have mutation tools advertised in ListTools. Calls would then fail noisily at the JumpCloud API layer with 403s, polluting the audit trail. Adds a profile-level role field (auth_profile_role: read_only) that the MCP server honors at startup: - config: ProfileRole(profile), IsReadOnlyProfile(), SetProfileRole with validation. Only ProfileRoleReadOnly ("read_only") is accepted. - mcp serve: applyProfileRole() helper coerces readOnly=true when the active profile carries the role; emits a one-line stderr warning if the operator hadn't already passed --read-only; and rejects the start with a clear error if --read-only=false was passed explicitly. - jc auth status: surfaces a 'Profile Role' line (and JSON field) when the active profile has a role set. Note: KLA-409's original Parts A (allow/block lists) and B (--read-only flag) were already shipped in earlier work; this is just the remaining Slice C (profile-level safety net). Tests: 4 new config tests, 4 new applyProfileRole helper tests, 2 new auth status output tests. All package suites green. Doc update for docs/AUTH.md will follow once PR #21 (KLA-410) lands; this PR is code-only to avoid a stacked-PR conflict. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/cmd/auth.go | 8 ++ internal/cmd/auth_test.go | 61 ++++++++++++++++ internal/cmd/mcp.go | 42 +++++++++++ internal/cmd/mcp_test.go | 72 ++++++++++++++++++ internal/config/config.go | 33 +++++++++ internal/config/config_test.go | 129 +++++++++++++++++++++++++++++++++ 6 files changed, 345 insertions(+) diff --git a/internal/cmd/auth.go b/internal/cmd/auth.go index 9d1ebe4..67b2754 100644 --- a/internal/cmd/auth.go +++ b/internal/cmd/auth.go @@ -394,6 +394,7 @@ func runAuthStatus(cmd *cobra.Command, args []string) error { Authenticated: false, Profile: profile, AuthMethod: authMethod, + ProfileRole: config.ProfileRole(profile), } if authMethod == "service_account" { @@ -457,6 +458,9 @@ func runAuthStatus(cmd *cobra.Command, args []string) error { if status.Authenticated { fmt.Fprintf(cmd.OutOrStdout(), "Authenticated: yes\n") fmt.Fprintf(cmd.OutOrStdout(), "Profile: %s\n", status.Profile) + if status.ProfileRole != "" { + fmt.Fprintf(cmd.OutOrStdout(), "Profile Role: %s\n", status.ProfileRole) + } fmt.Fprintf(cmd.OutOrStdout(), "Auth Method: %s\n", status.AuthMethod) fmt.Fprintf(cmd.OutOrStdout(), "Org Name: %s\n", status.OrgName) fmt.Fprintf(cmd.OutOrStdout(), "Org ID: %s\n", status.OrgID) @@ -471,6 +475,9 @@ func runAuthStatus(cmd *cobra.Command, args []string) error { } else { fmt.Fprintf(cmd.OutOrStdout(), "Authenticated: no\n") fmt.Fprintf(cmd.OutOrStdout(), "Profile: %s\n", status.Profile) + if status.ProfileRole != "" { + fmt.Fprintf(cmd.OutOrStdout(), "Profile Role: %s\n", status.ProfileRole) + } fmt.Fprintf(cmd.OutOrStdout(), "Auth Method: %s\n", status.AuthMethod) if status.AuthMethod == "service_account" { fmt.Fprintf(cmd.OutOrStdout(), "Run 'jc auth login --service-account' to re-authenticate.\n") @@ -486,6 +493,7 @@ func runAuthStatus(cmd *cobra.Command, args []string) error { type authStatusInfo struct { Authenticated bool `json:"authenticated"` Profile string `json:"profile"` + ProfileRole string `json:"profile_role,omitempty"` AuthMethod string `json:"auth_method"` OrgName string `json:"org_name,omitempty"` OrgID string `json:"org_id,omitempty"` diff --git a/internal/cmd/auth_test.go b/internal/cmd/auth_test.go index 304152d..10b3884 100644 --- a/internal/cmd/auth_test.go +++ b/internal/cmd/auth_test.go @@ -439,6 +439,67 @@ profiles: } } +func TestAuthStatus_ShowsProfileRoleWhenSet(t *testing.T) { + keyring.MockInit() + setupTestConfig(t, `active_profile: reporting +profiles: + reporting: + api_key: "" + auth_profile_role: read_only +`) + viper.Set("defaults.output", "table") + + ts := startMockJCServer(t, "org-ro", "Reporting Org", http.StatusOK) + defer ts.Close() + overrideAPIClient(t, ts.URL) + + viper.Set("api_key", "readonly-key-9876") + + cmd := &cobra.Command{} + stdout := new(bytes.Buffer) + cmd.SetOut(stdout) + cmd.SetErr(new(bytes.Buffer)) + + if err := runAuthStatus(cmd, nil); err != nil { + t.Fatalf("runAuthStatus() error: %v", err) + } + + got := stdout.String() + if !strings.Contains(got, "Profile Role: read_only") { + t.Errorf("expected 'Profile Role: read_only' in output, got %q", got) + } +} + +func TestAuthStatus_OmitsProfileRoleWhenUnset(t *testing.T) { + keyring.MockInit() + setupTestConfig(t, `active_profile: default +profiles: + default: + api_key: "" +`) + viper.Set("defaults.output", "table") + + ts := startMockJCServer(t, "org-norole", "No Role Org", http.StatusOK) + defer ts.Close() + overrideAPIClient(t, ts.URL) + + viper.Set("api_key", "test-key-norole-1234") + + cmd := &cobra.Command{} + stdout := new(bytes.Buffer) + cmd.SetOut(stdout) + cmd.SetErr(new(bytes.Buffer)) + + if err := runAuthStatus(cmd, nil); err != nil { + t.Fatalf("runAuthStatus() error: %v", err) + } + + got := stdout.String() + if strings.Contains(got, "Profile Role:") { + t.Errorf("did not expect 'Profile Role:' line when role is unset; got %q", got) + } +} + func TestAuthStatus_NotAuthenticated_ExitCode3(t *testing.T) { keyring.MockInit() setupTestConfig(t, `active_profile: default diff --git a/internal/cmd/mcp.go b/internal/cmd/mcp.go index e48894f..5df2866 100644 --- a/internal/cmd/mcp.go +++ b/internal/cmd/mcp.go @@ -113,6 +113,23 @@ Use JC_PROFILE environment variable to select which JumpCloud org to use.`, if !cmd.Flags().Changed("port") { port = config.MCPSSEPort() } + // Profile-role enforcement: a profile bound to a read-only OAuth + // client must not advertise mutation tools. Reject the start + // rather than silently coercing — operators who passed + // --read-only=false should see the error, not a phantom override. + coerced, warning, err := applyProfileRole( + config.ActiveProfile(), + config.IsReadOnlyProfile(), + cmd.Flags().Changed("read-only"), + readOnly, + ) + if err != nil { + return err + } + readOnly = coerced + if warning != "" { + fmt.Fprintln(cmd.ErrOrStderr(), warning) + } return runMcpServe(rateLimit, readOnly, transport, addr, port, corsOrigin, tlsCert, tlsKey, requireAuth) }, } @@ -178,6 +195,31 @@ func resolveSSEAddr(addr string, port int) string { return addr } +// applyProfileRole reconciles the --read-only flag with the active +// profile's role. Returns the effective read-only value, an optional +// warning message to surface to the operator, and an error if the flag +// and profile role conflict. +// +// Rules: +// - Profile is not read-only → pass through unchanged. +// - Profile is read-only and operator passed --read-only=false → error. +// - Profile is read-only and read-only is already true → no warning, +// no change (operator and profile agree). +// - Profile is read-only and read-only is false but flag wasn't +// explicitly set → coerce to true and emit a warning. +func applyProfileRole(activeProfile string, profileReadOnly, flagChanged, readOnly bool) (bool, string, error) { + if !profileReadOnly { + return readOnly, "", nil + } + if flagChanged && !readOnly { + return false, "", fmt.Errorf("active profile %q is read-only; --read-only=false is incompatible", activeProfile) + } + if readOnly { + return true, "", nil + } + return true, fmt.Sprintf("Profile %q is read-only — forcing --read-only and rejecting destructive tools.", activeProfile), nil +} + func runMcpServe(rateLimit int, readOnly bool, transport, addr string, port int, corsOrigin, tlsCert, tlsKey string, requireAuth bool) error { server := mcp.NewServer(mcp.Options{ RateLimit: rateLimit, diff --git a/internal/cmd/mcp_test.go b/internal/cmd/mcp_test.go index 01d5516..8ea84b1 100644 --- a/internal/cmd/mcp_test.go +++ b/internal/cmd/mcp_test.go @@ -324,3 +324,75 @@ func TestMcpCmd_IncludesToolsSubcommand(t *testing.T) { t.Error("expected mcp help to mention tools subcommand") } } + +func TestApplyProfileRole_NoRolePassesThrough(t *testing.T) { + cases := []struct { + flagChanged bool + readOnly bool + }{ + {flagChanged: false, readOnly: false}, + {flagChanged: false, readOnly: true}, + {flagChanged: true, readOnly: false}, + {flagChanged: true, readOnly: true}, + } + for _, c := range cases { + got, warn, err := applyProfileRole("default", false, c.flagChanged, c.readOnly) + if err != nil { + t.Errorf("unexpected error for %+v: %v", c, err) + } + if warn != "" { + t.Errorf("unexpected warning for %+v: %q", c, warn) + } + if got != c.readOnly { + t.Errorf("for %+v: got readOnly=%v, want %v", c, got, c.readOnly) + } + } +} + +func TestApplyProfileRole_ReadOnlyProfileForcesTrue(t *testing.T) { + got, warn, err := applyProfileRole("reporting", true, false, false) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !got { + t.Errorf("readOnly = false, want true (read-only profile, no flag)") + } + if !strings.Contains(warn, "reporting") { + t.Errorf("warning missing profile name: %q", warn) + } + if !strings.Contains(warn, "read-only") { + t.Errorf("warning missing read-only mention: %q", warn) + } +} + +func TestApplyProfileRole_ReadOnlyProfileSilentWhenFlagAgrees(t *testing.T) { + got, warn, err := applyProfileRole("reporting", true, true, true) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !got { + t.Errorf("readOnly = false, want true") + } + if warn != "" { + t.Errorf("expected no warning when operator and profile agree, got %q", warn) + } +} + +func TestApplyProfileRole_ReadOnlyProfileRejectsExplicitFalse(t *testing.T) { + got, warn, err := applyProfileRole("reporting", true, true, false) + if err == nil { + t.Fatal("expected error when --read-only=false is passed against a read-only profile") + } + if !strings.Contains(err.Error(), "reporting") { + t.Errorf("error missing profile name: %v", err) + } + if !strings.Contains(err.Error(), "incompatible") { + t.Errorf("error should call out the incompatibility: %v", err) + } + if got { + t.Errorf("on error, readOnly should not be coerced to true; got %v", got) + } + if warn != "" { + t.Errorf("on error, warning should be empty; got %q", warn) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 67b4933..e23cedb 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -203,6 +203,39 @@ func AuthMethod() string { return "api_key" } +// ProfileRoleReadOnly is the role value that constrains a profile to +// read-only operations. When the active profile carries this role, the +// MCP server boots with ReadOnly=true regardless of the --read-only flag. +const ProfileRoleReadOnly = "read_only" + +// ProfileRole returns the role assigned to the named profile. An empty +// string means the profile has no role and behaves normally. The only +// non-empty value currently understood is ProfileRoleReadOnly. +func ProfileRole(profile string) string { + if profile == "" { + profile = ActiveProfile() + } + return viper.GetString("profiles." + profile + ".auth_profile_role") +} + +// IsReadOnlyProfile returns true if the active profile is bound to +// read-only operations. Used by `jc mcp serve` to refuse mutation tools +// even if the operator forgot the --read-only flag. +func IsReadOnlyProfile() bool { + return ProfileRole("") == ProfileRoleReadOnly +} + +// SetProfileRole writes the role onto the named profile. Pass an empty +// role to clear it. Only ProfileRoleReadOnly (or "") is accepted today; +// unknown values return an error so the config doesn't carry typos that +// silently fail to enforce anything. +func SetProfileRole(profile, role string) error { + if role != "" && role != ProfileRoleReadOnly { + return fmt.Errorf("invalid profile role %q (must be %q or empty)", role, ProfileRoleReadOnly) + } + return SetProfileField(profile, "auth_profile_role", role) +} + // ClientID returns the OAuth 2.0 client ID for the active profile. func ClientID() string { profile := ActiveProfile() diff --git a/internal/config/config_test.go b/internal/config/config_test.go index cbc60e4..6e07168 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1350,3 +1350,132 @@ func TestNoColor_FlagOverridesTTY(t *testing.T) { t.Error("NoColor() = false, want true when --no-color flag is set even with TTY") } } + +func TestProfileRole_DefaultEmpty(t *testing.T) { + resetViper() + defer resetViper() + + tmp := t.TempDir() + dir := filepath.Join(tmp, "jc") + cfgPath := filepath.Join(dir, "config.yaml") + t.Setenv("JC_CONFIG", cfgPath) + t.Setenv("JC_PROFILE", "") + + _ = os.MkdirAll(dir, 0700) + _ = os.WriteFile(cfgPath, []byte(`active_profile: default +profiles: + default: + api_key: "" +`), 0600) + + if err := Init(); err != nil { + t.Fatalf("Init() error: %v", err) + } + + if got := ProfileRole(""); got != "" { + t.Errorf("ProfileRole('') = %q, want empty", got) + } + if IsReadOnlyProfile() { + t.Error("IsReadOnlyProfile() = true on a profile with no role") + } +} + +func TestProfileRole_ReadOnly(t *testing.T) { + resetViper() + defer resetViper() + + tmp := t.TempDir() + dir := filepath.Join(tmp, "jc") + cfgPath := filepath.Join(dir, "config.yaml") + t.Setenv("JC_CONFIG", cfgPath) + t.Setenv("JC_PROFILE", "") + + _ = os.MkdirAll(dir, 0700) + _ = os.WriteFile(cfgPath, []byte(`active_profile: reporting +profiles: + reporting: + api_key: "" + auth_profile_role: read_only + admin: + api_key: "" +`), 0600) + + if err := Init(); err != nil { + t.Fatalf("Init() error: %v", err) + } + + if got := ProfileRole("reporting"); got != ProfileRoleReadOnly { + t.Errorf("ProfileRole('reporting') = %q, want %q", got, ProfileRoleReadOnly) + } + if !IsReadOnlyProfile() { + t.Error("IsReadOnlyProfile() = false on the active read-only profile") + } + if got := ProfileRole("admin"); got != "" { + t.Errorf("ProfileRole('admin') = %q, want empty", got) + } +} + +func TestSetProfileRole_Valid(t *testing.T) { + resetViper() + defer resetViper() + + tmp := t.TempDir() + dir := filepath.Join(tmp, "jc") + cfgPath := filepath.Join(dir, "config.yaml") + t.Setenv("JC_CONFIG", cfgPath) + t.Setenv("JC_PROFILE", "") + + _ = os.MkdirAll(dir, 0700) + _ = os.WriteFile(cfgPath, []byte(`active_profile: default +profiles: + default: + api_key: "" +`), 0600) + + if err := Init(); err != nil { + t.Fatalf("Init() error: %v", err) + } + + if err := SetProfileRole("default", ProfileRoleReadOnly); err != nil { + t.Fatalf("SetProfileRole(read_only) error: %v", err) + } + if got := ProfileRole("default"); got != ProfileRoleReadOnly { + t.Errorf("after SetProfileRole(read_only): role = %q, want %q", got, ProfileRoleReadOnly) + } + + // Clear the role. + if err := SetProfileRole("default", ""); err != nil { + t.Fatalf("SetProfileRole('') error: %v", err) + } + if got := ProfileRole("default"); got != "" { + t.Errorf("after SetProfileRole(''): role = %q, want empty", got) + } +} + +func TestSetProfileRole_Invalid(t *testing.T) { + resetViper() + defer resetViper() + + tmp := t.TempDir() + dir := filepath.Join(tmp, "jc") + cfgPath := filepath.Join(dir, "config.yaml") + t.Setenv("JC_CONFIG", cfgPath) + + _ = os.MkdirAll(dir, 0700) + _ = os.WriteFile(cfgPath, []byte(`active_profile: default +profiles: + default: + api_key: "" +`), 0600) + + if err := Init(); err != nil { + t.Fatalf("Init() error: %v", err) + } + + if err := SetProfileRole("default", "admin"); err == nil { + t.Error("SetProfileRole('admin') accepted unknown role; want error") + } + if got := ProfileRole("default"); got != "" { + t.Errorf("ProfileRole after rejected SetProfileRole = %q, want empty", got) + } +}