feat(extensions): Add extension permissions#691
Conversation
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
76326bd to
5c8e59a
Compare
a757c03 to
6013840
Compare
|
Thank you @debsmita1, it looks great! I have two requests:
|
6013840 to
63afb2f
Compare
63afb2f to
e66e1eb
Compare
PatAKnight
left a comment
There was a problem hiding this comment.
Left a couple of initial comments but will need to do a deeper dive into the plugin itself to try to understand it a bit better. If you want, we can sync up on this as I recently had to do this for the RBAC plugin and it is still fairly top of mind.
There was a problem hiding this comment.
I wonder if this should just be the read permission only and then we can use the create permission for post.
There was a problem hiding this comment.
@PatAKnight I updated this to create a separate service to determine the privileges, which is then consumed by the UI to show "Install" or "View" plugin configuration based on the access rights
And I converted this one to check only the read access, and eventually it should return the YAML config
PTAL and let me know your thoughts on this
@ShiranHi Addressed your comments and updated the GIF |
285f57c to
6211a8b
Compare
6211a8b to
4bf1c56
Compare
|
| }), | ||
| apply: (plugin: MarketplacePlugin, { pluginNames }) => { | ||
| return pluginNames && pluginNames.length > 0 | ||
| ? !!pluginNames?.find( |
There was a problem hiding this comment.
super nit. isn't this the same?
| ? !!pluginNames?.find( | |
| ? pluginNames?.some( |
There was a problem hiding this comment.
Pull Request Overview
This PR adds extension permissions to the marketplace plugins by introducing new API endpoints, permission rules, and updated hooks/components to support permission-based access for installing and configuring extensions. Key changes include:
- Adding a new hook (useExtensionConfigPermission) and associated API methods for extension configuration authorization.
- Updating the marketplace backend router to handle new permission endpoints for reading and installing plugin configurations.
- Introducing permission rules and updating dependency versions to support the new permission framework.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| workspaces/marketplace/plugins/marketplace/src/hooks/useExtensionConfigPermission.ts | New hook to fetch plugin configuration authorization. |
| workspaces/marketplace/plugins/marketplace/src/components/MarketplacePluginInstallContent.tsx | Updated installation UI with permission-based disable logic and tooltips. |
| workspaces/marketplace/plugins/marketplace/src/components/MarketplacePluginContent.tsx | Refactored action button to use permission hook and tooltip. |
| workspaces/marketplace/plugins/marketplace-backend/src/router.ts | Added endpoints for plugin configuration authorization and configuration access with conditional permissions. |
| workspaces/marketplace/plugins/marketplace-backend/src/permissions/rules.ts | New permission rules for extensions. |
| Various package.json and common files | Added new dependencies and API definitions to support extension permissions. |
christoph-jerolimov
left a comment
There was a problem hiding this comment.
Some thoughts, please take a look. I'm fine if we want align some of these also later:
| MarketplacePlugin, | ||
| ExtentionFilter | ||
| >().with({ | ||
| pluginId: 'marketplace', |
There was a problem hiding this comment.
Next step is to rename the plugin, but we can use the new pluginId here already.
Please change it also in plugins/marketplace/src/plugin.ts
| pluginId: 'marketplace', | |
| pluginId: 'extensions', |
| .string() | ||
| .array() | ||
| .optional() | ||
| .describe('List of plugin names to match on'), |
There was a problem hiding this comment.
| .describe('List of plugin names to match on'), | |
| .describe('List of plugin names or titles to match on'), |
| const [readDecision, installDecision] = await Promise.all([ | ||
| authorizeConditional(req, extensionPluginReadPermission), | ||
| authorizeConditional(req, extensionPluginCreatePermission), | ||
| ]); | ||
| if ( | ||
| readDecision.result === AuthorizeResult.DENY && | ||
| installDecision.result === AuthorizeResult.DENY | ||
| ) { | ||
| res.status(403); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Isn't it cleaner if the /authorize call return always a successful response but with an access status?
Like
200 OK
{
read: "ALLOW"
write: "DENY"
}
Or whatever you want return.
Because I see that the MarketplaceClient expects 200 and converts this into an error.
But you don't handle errors when using useExtensionConfigPermission in MarketplacePluginContent.
| if (authorizedActions.length === 0) { | ||
| res.status(403); | ||
| return; | ||
| } |
| res.status(403).json({ error: 'Forbidden' }); | ||
| return; |
There was a problem hiding this comment.
Can we throw an error instead? For all 403 cases.
import {
NotAllowedError,
} from '@backstage/errors';| res.status(403).json({ error: 'Forbidden' }); | |
| return; | |
| throw new NotAllowedError(`Not allowed to configure ${req.params.namespace}:${req.params.name}`); |
| /** This permission grants access to the endpoint that reads the configuration of the extension plugin | ||
| * @public | ||
| */ | ||
| export const extensionPluginReadPermission = createPermission({ |
There was a problem hiding this comment.
The variable name should include Configuration as well, to avoid confusing that this is a permission to read the plugin metadata.
IMHO, as long as we don't differentiate between Create update and Delete we should have two permissions:
- read
- write or update
If you want have separate for enableing and disabling the plugin I personally would use
- enable
- disable
Actually, that's then maybe not a configuration enable/disable, its then maybe 'extensions.plugin.enable.
But I think we could start here also with extensions.plugin.configuration.write, because it just does this at the moment.
| const params = useRouteRefParams(pluginRouteRef); | ||
| const getIndexPath = useRouteRef(rootRouteRef); | ||
| const getInstallPath = useRouteRef(pluginInstallRouteRef); | ||
| const canInstallPlugin = useExtensionConfigPermission( |
There was a problem hiding this comment.
I think this hook should include that its for plugins
| const canInstallPlugin = useExtensionConfigPermission( | |
| const pluginConfigPerm = usePluginConfigurationPermissions( |
| > | ||
| Reset | ||
| </Button> | ||
| {canInstallPlugin.data?.authorizedActions?.includes('create') && ( |
There was a problem hiding this comment.
| {canInstallPlugin.data?.authorizedActions?.includes('create') && ( | |
| {pluginConfigPermissions.data?.includes('create') && ( |
|
|
||
| import { useMarketplaceApi } from './useMarketplaceApi'; | ||
|
|
||
| export const useExtensionConfigPermission = ( |
There was a problem hiding this comment.
other hook names are aligned with the MarketplaceApi function they call and just replace get with use
christoph-jerolimov
left a comment
There was a problem hiding this comment.
As discussed, approving this spike but it would be nice if we can address some reviews soon. Esp. renaming all permission names to extensions.* before we release the plugin.
I will set the release on hold until this changes was done!
Addressed the review comments here #762 |
* feat(marketplace): add permissions to extensions * adding conditional policies * add new service to check config authorization



Hey, I just made a Pull Request!
Resolves:
https://issues.redhat.com/browse/RHIDP-6766
How to test:
Install rbac frontend and backend plugins
Add the Github signin page
Add the following in you rbac-policy.csv file
Add the following to your
app-config.yamlScreenshots/GIF:
User with no config read permission
User with only config read permission
Screen.Recording.2025-04-30.at.4.43.57.PM.mov
User with both read and create permissions
Screen.Recording.2025-04-28.at.9.51.56.PM.mov
User with conditional permissions
Screen.Recording.2025-04-28.at.9.53.22.PM.mov
TODO (to be addressed in separate PRs):
✔️ Checklist