Plugins access control #3486
Plugins access control #3486
Conversation
|
@macobo I'm unfortunately going to have to put this on your lap as well. I have reviewed the frontend and all seems good there! Yet I did not manage to go through any |
frontend/src/lib/utils.tsx
Outdated
| @@ -747,3 +747,14 @@ export function sortedKeys(object: Record<string, any>): Record<string, any> { | |||
| } | |||
| return newObject | |||
| } | |||
|
|
|||
| export function endWithPeriod(text?: string | null): string { | |||
There was a problem hiding this comment.
Suggestion: endWithPunctation.
| <TabPane tab="Repository" key={PluginTab.Repository}> | ||
| <RepositoryTab /> | ||
| </TabPane> | ||
| {user.organization?.plugins_access_level === PluginsAccessLevel.Root && ( |
There was a problem hiding this comment.
This code is not self-evident, it's not clear to a reader what plugin_access_level stands for or what the right levels are.
Suggestion:
Extract module frontend/src/scenes/plugins/accessControl.ts containing function(s) like hasAccessToPlugins(user), canInstallPlugins(user) etc. This keeps the business logic centralized and the component files readable.
| @@ -170,6 +173,35 @@ export function PluginDrawer(): JSX.Element { | |||
| </div> | |||
| ) : null} | |||
|
|
|||
| {user?.organization?.plugins_access_level === PluginsAccessLevel.Root && | |||
| user?.is_multi_tenancy && ( | |||
There was a problem hiding this comment.
Q: Will this mean that this is a cloud user or...?
Also: Extract function canEnableGlobalPlugins.
There was a problem hiding this comment.
Didn't extract the is_multi_tenancy since it'd require passing in user too and it's not globally relevant to permissioning. Just a presentation thing for now. Did extract the Root (etc.) check though.
| <GlobalOutlined /> Managed by {organization_name} | ||
| </Tag> | ||
| )} | ||
| {user?.organization?.id === organization_id && |
There was a problem hiding this comment.
Suggestion: canManage(user, plugin)
| @@ -112,11 +114,9 @@ export const pluginsLogic = kea< | |||
| return { ...plugins, [id]: response } | |||
| }, | |||
| updatePlugin: async ({ id }) => { | |||
| const { plugins } = values | |||
| const response = await api.update(`api/organizations/@current/plugins/${id}`, {}) | |||
| const response = await api.create(`api/organizations/@current/plugins/${id}/upgrade`) | |||
There was a problem hiding this comment.
Note: I find the update/upgrade vs patch semantics a bit weird, but not sure how to improve this.
There was a problem hiding this comment.
(blame Marius) Well initially PATCH was responsible for triggering this, but considering PATCH is represented as update in our API helper and in DRF, it is confusing. Had to change the endpoint to a separate /upgrade, but in the UI the feature is still "update".
There was a problem hiding this comment.
Agreed. This is non-blocking imo.
* Add Organization.PluginsAccess * Rename PluginsAccess to PluginsAccessLevel * Use Organization.plugins_access_level in can_…_plugins_via_api * Add migration for Organization.plugins_access_level * Remove unused PLUGINS_CLOUD_WHITELISTED_ORG_IDS * Update access.py * Add OrganizationPluginsAccessLevel TS enum * Fix merge * Disable LocalPlugin UI on Cloud * Move away from PluginAccess interface * Extend PluginsAccessLevel range * Refactor PluginsAccessLevel for brevity * Remove PluginAccess interface completely * Add plugins managed globally * Update migration * Show managing org name in "Managed" plugin tag * Smoothen some rough edges * Smoothen more edges * Restore correct MULTI_TENANCY default * All the edges * Fix most existing tests * Remove PLUGINS_*_VIA_API env var support * Update pluginsNeedingUpdates * Remove can_*_plugins_via_api from instance status page * Add tests and polish permissioning * Update migration * Fix typing * Make plugin drawer UI less intrusive * Update migration * Fix Uninstall button condition * Use unified _preflight status endpoint instead of the custom plugins one * Fix plugin update label condition * Fix "Check for updates" button condition * Explain PluginsAccessLevel choices with comments * Hide global plugin installation option on self-hosted * Don't actions.loadRepository() as install org * Improve permissioning with tests * Satisfy mypy * Add plugins access level to admin and fix org admin * Check plugins access level more * Rename endWithPeriod * Refactor FE access control checks to accessControl.ts * Deduplicate permissioning * Add exception message * Align backend and frontend plugins access level helpers * Add plugins access level helper tests * Fix ChartFilter
Changes
Basically, resolves #3328. Context in the issue.
Checklist