-
Notifications
You must be signed in to change notification settings - Fork 3
Disable connection to unsupported IdP #172
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
Conversation
| additionalText={ | ||
| currentContext === context.name ? '(default IdP)' : undefined | ||
| } | ||
| additionalText={`(${ |
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.
please use the mcp resource and check the spec.authentication.enableSystemIdentityProvider === true
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.
Changed as discussed
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 adds logic to disable connections when the system identity provider (IdP) is not enabled and surface a warning, and updates the ConnectButton to disable non-default IdP contexts with proper translation keys.
- Fetches the managed control plane resource to check
enableSystemIdentityProvider, disables the Connect button if false, and shows an Infobox warning. - Updates
ConnectButtonto disable menu items for non-default IdPs, using translation keys for “default” and “unsupported” IdP labels. - Adds
defaultIdPandunsupportedIdPentries to the English locale file.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/components/ControlPlanes/ControlPlaneCard/ControlPlaneCard.tsx | Import useResource to fetch config, disable Connect when IdP off, and show warning Infobox |
| src/components/ControlPlanes/ConnectButton.tsx | Disable non-openmcp contexts, update additionalText to use translations |
| public/locales/en.json | Add defaultIdP and unsupportedIdP translation strings |
Comments suppressed due to low confidence (2)
src/components/ControlPlanes/ControlPlaneCard/ControlPlaneCard.tsx:137
- Add a unit or integration test to verify that the warning Infobox is displayed when
enableSystemIdentityProvideris disabled and the Connect button is rendered.
{showWarningBecauseOfDisabledSystemIdentityProvider && (
src/components/ControlPlanes/ConnectButton.tsx:110
- Add tests to ensure that menu items for non-
openmcpcontexts are correctly disabled and that the appropriate additional text is shown.
disabled={context.context.user !== 'openmcp'}
| import useResource, { | ||
| useApiResourceMutation, | ||
| } from '../../../lib/api/useApiResource.ts'; |
Copilot
AI
Jul 10, 2025
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.
The imported useApiResourceMutation is not used in this file. Remove it to clean up unused imports.
| import useResource, { | |
| useApiResourceMutation, | |
| } from '../../../lib/api/useApiResource.ts'; | |
| import useResource from '../../../lib/api/useApiResource.ts'; |
| // @ts-ignore | ||
| !!controlPlaneConfig.data?.spec?.authentication |
Copilot
AI
Jul 10, 2025
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.
Avoid using // @ts-ignore to suppress type errors. Consider updating the type definitions for controlPlaneConfig.data.spec.authentication so you can safely access enableSystemIdentityProvider without ignoring TypeScript.
| // @ts-ignore | |
| !!controlPlaneConfig.data?.spec?.authentication | |
| !!(controlPlaneConfig.data as ControlPlaneConfigData)?.spec?.authentication |
| context.context.user === 'openmcp' | ||
| ? t('ConnectButton.defaultIdP') | ||
| : t('ConnectButton.unsupportedIdP') | ||
| })`} | ||
| disabled={context.context.user !== 'openmcp'} |
Copilot
AI
Jul 10, 2025
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.
[nitpick] The string 'openmcp' is used directly to detect the system IdP. Consider extracting this into a named constant to avoid magic strings and improve readability.
| context.context.user === 'openmcp' | |
| ? t('ConnectButton.defaultIdP') | |
| : t('ConnectButton.unsupportedIdP') | |
| })`} | |
| disabled={context.context.user !== 'openmcp'} | |
| context.context.user === SYSTEM_IDP | |
| ? t('ConnectButton.defaultIdP') | |
| : t('ConnectButton.unsupportedIdP') | |
| })`} | |
| disabled={context.context.user !== SYSTEM_IDP} |
Closes openmcp-project/backlog#143