-
Notifications
You must be signed in to change notification settings - Fork 0
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
dashboard admin(groups/users) implementation and integrating with dynamic application config #303
Changes from 10 commits
72fc938
29236e3
22323c1
f9666a0
123c87e
166451a
363aeaa
c8bb0b5
74d9a26
a07dde0
518cd75
dc77999
71c20d9
c61dac9
0bf41b3
f0c1b5d
2018bdf
979851a
3a70922
a69cc58
7d83f48
f50093f
e9fd21a
909af1c
b342950
27d95fe
acfb166
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -11,16 +11,21 @@ import { | |||||||||
Plugin, | ||||||||||
Logger, | ||||||||||
CoreStart, | ||||||||||
OpenSearchDashboardsRequest, | ||||||||||
} from '../../../core/server'; | ||||||||||
import { | ||||||||||
WORKSPACE_SAVED_OBJECTS_CLIENT_WRAPPER_ID, | ||||||||||
WORKSPACE_CONFLICT_CONTROL_SAVED_OBJECTS_CLIENT_WRAPPER_ID, | ||||||||||
} from '../common/constants'; | ||||||||||
import { IWorkspaceClientImpl } from './types'; | ||||||||||
import { AppPluginSetupDependencies, IWorkspaceClientImpl } from './types'; | ||||||||||
import { WorkspaceClient } from './workspace_client'; | ||||||||||
import { registerRoutes } from './routes'; | ||||||||||
import { WorkspaceSavedObjectsClientWrapper } from './saved_objects'; | ||||||||||
import { cleanWorkspaceId, getWorkspaceIdFromUrl } from '../../../core/server/utils'; | ||||||||||
import { | ||||||||||
cleanWorkspaceId, | ||||||||||
getWorkspaceIdFromUrl, | ||||||||||
updateWorkspaceState, | ||||||||||
} from '../../../core/server/utils'; | ||||||||||
import { WorkspaceConflictSavedObjectsClientWrapper } from './saved_objects/saved_objects_wrapper_for_check_workspace_conflict'; | ||||||||||
import { | ||||||||||
SavedObjectsPermissionControl, | ||||||||||
|
@@ -55,12 +60,98 @@ export class WorkspacePlugin implements Plugin<{}, {}> { | |||||||||
}); | ||||||||||
} | ||||||||||
|
||||||||||
private isRequestByDashboardAdmin( | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can be a util function inside another file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have moved to src/plugins/workspace/server/utils.ts and add unit test. |
||||||||||
request: OpenSearchDashboardsRequest, | ||||||||||
groups: string[], | ||||||||||
users: string[], | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I'd recommend combine these two params into a param named principals: Principals. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think using deconstructed code is more conducive to subsequent use. |
||||||||||
configGroups: string[], | ||||||||||
configUsers: string[] | ||||||||||
) { | ||||||||||
if (configGroups.length === 0 && configUsers.length === 0) { | ||||||||||
updateWorkspaceState(request, { | ||||||||||
isDashboardAdmin: false, | ||||||||||
}); | ||||||||||
return; | ||||||||||
} | ||||||||||
const groupMatchAny = groups.some((group) => configGroups.includes(group)) || false; | ||||||||||
const userMatchAny = users.some((user) => configUsers.includes(user)) || false; | ||||||||||
updateWorkspaceState(request, { | ||||||||||
isDashboardAdmin: groupMatchAny || userMatchAny, | ||||||||||
}); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the function name, I do not think it appropriate to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I will change the function name to |
||||||||||
} | ||||||||||
|
||||||||||
private async setupPermission( | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to declare as a async function here I think |
||||||||||
core: CoreSetup, | ||||||||||
config: WorkspacePluginConfigType, | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we leverage the existing property There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||||||||||
{ applicationConfig }: AppPluginSetupDependencies | ||||||||||
) { | ||||||||||
this.permissionControl = new SavedObjectsPermissionControl(this.logger); | ||||||||||
|
||||||||||
core.http.registerOnPostAuth(async (request, response, toolkit) => { | ||||||||||
let groups: string[]; | ||||||||||
let users: string[]; | ||||||||||
|
||||||||||
// There may be calls to saved objects client before user get authenticated, need to add a try catch here as `getPrincipalsFromRequest` will throw error when user is not authenticated. | ||||||||||
try { | ||||||||||
({ groups = [], users = [] } = this.permissionControl!.getPrincipalsFromRequest(request)); | ||||||||||
} catch (e) { | ||||||||||
return toolkit.next(); | ||||||||||
} | ||||||||||
if (groups.length === 0 && users.length === 0) { | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the cluster does not require authentication, anyone can be dashboardAdmin I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, a bit like OSD without security plugin installed that will have almost admin permissions. Also want to hear other's thoughts. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean to update isDashboardAdmin state to true if |
||||||||||
updateWorkspaceState(request, { | ||||||||||
isDashboardAdmin: false, | ||||||||||
}); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If users/groups is null, we can prevent the program from running down in advance. Meanwhile, configGroups/configUsers has not been obtained. |
||||||||||
return toolkit.next(); | ||||||||||
} | ||||||||||
|
||||||||||
this.logger.info('Dynamic application configuration enabled:' + !!applicationConfig); | ||||||||||
if (!!applicationConfig) { | ||||||||||
const [coreStart] = await core.getStartServices(); | ||||||||||
const scopeClient = coreStart.opensearch.client.asScoped(request); | ||||||||||
const applicationConfigClient = applicationConfig.getConfigurationClient(scopeClient); | ||||||||||
|
||||||||||
const [configGroups, configUsers] = await Promise.all([ | ||||||||||
applicationConfigClient | ||||||||||
.getEntityConfig('workspace.dashboardAdmin.groups') | ||||||||||
.catch(() => undefined), | ||||||||||
applicationConfigClient | ||||||||||
.getEntityConfig('workspace.dashboardAdmin.users') | ||||||||||
.catch(() => undefined), | ||||||||||
]); | ||||||||||
|
||||||||||
this.isRequestByDashboardAdmin( | ||||||||||
request, | ||||||||||
groups, | ||||||||||
users, | ||||||||||
configGroups ? [configGroups] : [], | ||||||||||
configUsers ? [configUsers] : [] | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
configGroups and configUsers should already be array I think? |
||||||||||
); | ||||||||||
return toolkit.next(); | ||||||||||
} | ||||||||||
|
||||||||||
const configGroups = config.dashboardAdmin.groups || []; | ||||||||||
const configUsers = config.dashboardAdmin.users || []; | ||||||||||
this.isRequestByDashboardAdmin(request, groups, users, configGroups, configUsers); | ||||||||||
return toolkit.next(); | ||||||||||
}); | ||||||||||
|
||||||||||
this.workspaceSavedObjectsClientWrapper = new WorkspaceSavedObjectsClientWrapper( | ||||||||||
this.permissionControl | ||||||||||
); | ||||||||||
|
||||||||||
core.savedObjects.addClientWrapper( | ||||||||||
0, | ||||||||||
WORKSPACE_SAVED_OBJECTS_CLIENT_WRAPPER_ID, | ||||||||||
this.workspaceSavedObjectsClientWrapper.wrapperFactory | ||||||||||
); | ||||||||||
} | ||||||||||
|
||||||||||
constructor(initializerContext: PluginInitializerContext) { | ||||||||||
this.logger = initializerContext.logger.get('plugins', 'workspace'); | ||||||||||
this.config$ = initializerContext.config.create<WorkspacePluginConfigType>(); | ||||||||||
} | ||||||||||
|
||||||||||
public async setup(core: CoreSetup) { | ||||||||||
public async setup(core: CoreSetup, { applicationConfig }: AppPluginSetupDependencies) { | ||||||||||
this.logger.debug('Setting up Workspaces service'); | ||||||||||
const config: WorkspacePluginConfigType = await this.config$.pipe(first()).toPromise(); | ||||||||||
const isPermissionControlEnabled = | ||||||||||
|
@@ -80,19 +171,7 @@ export class WorkspacePlugin implements Plugin<{}, {}> { | |||||||||
); | ||||||||||
|
||||||||||
this.logger.info('Workspace permission control enabled:' + isPermissionControlEnabled); | ||||||||||
if (isPermissionControlEnabled) { | ||||||||||
this.permissionControl = new SavedObjectsPermissionControl(this.logger); | ||||||||||
|
||||||||||
this.workspaceSavedObjectsClientWrapper = new WorkspaceSavedObjectsClientWrapper( | ||||||||||
this.permissionControl | ||||||||||
); | ||||||||||
|
||||||||||
core.savedObjects.addClientWrapper( | ||||||||||
0, | ||||||||||
WORKSPACE_SAVED_OBJECTS_CLIENT_WRAPPER_ID, | ||||||||||
this.workspaceSavedObjectsClientWrapper.wrapperFactory | ||||||||||
); | ||||||||||
} | ||||||||||
if (isPermissionControlEnabled) this.setupPermission(core, config, { applicationConfig }); | ||||||||||
|
||||||||||
registerRoutes({ | ||||||||||
http: core.http, | ||||||||||
|
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 naming
workspace.dashboardAdmin
may confuse me, it sounds like the admin for dashboard type only. I come few names likeworkspace.dashboardsAdmin
,workspace.admin
orworkpsace.superAdmin
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.
workspace.admin may be confusing as
admin of workspace
stands for the the users who have write permission on specific workspaces, vote for superAdmin.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.
If there is no configuration named
workspace.admin
, wouldworkspace.superAdmin
also be confused?