-
Notifications
You must be signed in to change notification settings - Fork 3
Edit MCP's resources improvements #347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the resource editing experience in the MCP (Managed Control Plane) space by implementing role-based access control and improving user feedback. It introduces admin rights checking to control edit permissions, adds edit buttons in YAML preview mode, and provides tooltips explaining why Flux-managed resources cannot be edited.
Key changes:
- Implemented role-based authorization checking via
hasMCPAdminRightsflag derived from MCP user role bindings - Added edit buttons in YAML preview panels for resources (visible only to admin users)
- Enhanced YAML editor configuration with Kubernetes schema validation and improved Monaco editor settings
Reviewed Changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/spaces/mcp/pages/McpPage.tsx |
Passes MCP role bindings to AuthProviderMcp for authorization checks |
src/spaces/mcp/auth/AuthContextMcp.tsx |
Implements admin rights checking logic based on user email and role bindings |
src/lib/monaco.ts |
Configures Monaco YAML with Kubernetes schema for validation and autocomplete |
src/lib/api/types/crate/controlPlanes.ts |
Adds TypeScript types for authorization and role bindings; updates jq query |
src/components/YamlEditor/YamlEditor.tsx |
Refactors inline styles to CSS modules and enhances editor options |
src/components/YamlEditor/YamlEditor.module.css |
New CSS module for YamlEditor component styling |
src/components/Yaml/YamlViewButton.tsx |
Adds support for custom toolbar content in YAML preview |
src/components/Yaml/YamlSidePanel.tsx |
Renders custom toolbar content when provided |
src/components/Yaml/YamlSidePanel.module.css |
Updates padding and border-radius styling |
src/components/Wizards/CreateManagedControlPlane/YamlSummarize.tsx |
Renames component from YamlPanel and fixes import paths |
src/components/Wizards/CreateManagedControlPlane/SummarizeStep.tsx |
Updates import to use renamed YamlSummarize component |
src/components/ControlPlane/ProvidersConfig.tsx |
Adds edit button to YAML preview with admin rights check |
src/components/ControlPlane/ManagedResources.tsx |
Adds edit button with Flux-managed resource detection and tooltip |
src/components/ControlPlane/Kustomizations.tsx |
Adds edit button to YAML preview with admin rights check |
src/components/ControlPlane/GitRepositories.tsx |
Adds edit button to YAML preview with admin rights check |
src/components/ControlPlane/ActionsMenu.tsx |
Adds tooltip property support for action menu items |
public/locales/en.json |
Adds translation keys for edit button, validation errors, and Flux-managed tooltip |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this PR together!
From a quick look, it adds the hasMCPAdminRights role info to the AuthProviderMcp. I'm a bit concerned that this mixes two different concepts: authentication (IdP) and authorization (MCP roles). What do you think about keeping them separate?
For example, create a new, dedicated context just for this MCP authorization info or for now (since we don’t have prop drilling issues) just calculate the admin rights in the parent McpPage (maybe with a hook?) and pass it down. That might be even easier.
This would help keep the AuthContext focused on its core responsibility.
Let me know your thoughts!
I extracted hasMCPAdminRights to a separate hook to keep them separate: |
What this PR does / why we need it: