-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: Knowledge workflow permission #4349
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
feat: Knowledge workflow permission #4349
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| }), | ||
| }, | ||
| redirect: (menu: any) => { | ||
| const from = 'workspace' |
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.
Some potential issues with the provided React component code:
- The
permissionarray is unnecessarily complex and verbose. - There are duplicated checks and permissions in different parts of the array.
- Some route parameters are incorrectly written (
folderIdshould betype,extensionIdseems misplaced).
Optimization suggestions:
- Combine similar conditions and roles into fewer function calls and simpler logic.
- Simplify repeated permission calculations.
- Correct inconsistencies such as
"shared"vs.'shared'.
Here's an optimized version:
const DocumentRouter = {
permission: [
(to) => {
if (to.params.type === '4') {
if (to.params.folderId === 'workspace-management') {
return new ComplexPermission([RoleConst.ADMIN], [], [], 'AND');
} else {
return RoleConst.WORKSPACE_MANAGE.getWorkspaceRole();
}
}
// Handle other types or folderIds appropriately
},
(to) => {
if (to.params.type === '4') {
return PermissionConst.KNOWLEDGE_WORKFLOW_READ.getKnowledgeWorkspaceResourcePermission(to.params.id);
}
<|fim_suffix|>n to.params.type.includes('document') && !to.params.extensionId;
};
DocumentRouter.redirect = ({ type }) => {
if (!['admin', 'editor'].includes(type)) return '/login';
};These improvements enhance readability, reduce redundancy, and handle various scenarios more efficiently.
| ) | ||
| RESOURCE_KNOWLEDGE_DOCUMENT_READ = Permission( | ||
| group=Group.SYSTEM_RES_KNOWLEDGE_DOCUMENT, operate=Operate.READ, role_list=[RoleConstants.ADMIN], | ||
| parent_group=[SystemGroup.RESOURCE_KNOWLEDGE], is_ee=settings.edition == "EE" |
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.
There are no apparent issues with the provided code snippet. It seems to be correctly defining enumerations and permission constants based on different groups and their associated operations, roles, and resource permissions. A few notes:
-
The new
KNOWLEDGE_WORKFLOWgroup is added along with its respective permissions (READ,EDIT) for both workspace and system-level access. -
Similar additional entries have been created for workflow-related permissions in system-level knowledge resources and user/shared workspaces.
-
There might be redundancy in the list of
resource_permission_group_listsince some names like "KNOWLEDGE_MANGE" and "KNOWLEDGE_VIEW" are repeated, but this doesn't affect the functionality unless those specific values need customization.
Overall, the structure looks clean and follows good naming conventions. If there's any further modification needed beyond these entries, please specify!
| IS_CE: new Edition('X-PACK-CE'), | ||
| } | ||
| export {PermissionConst, RoleConst, EditionConst} | ||
|
|
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 current code appears to be adding two new permissions for knowledge workflow access: KNOWLEDGE_WORKFLOW_READ and KNOWLEDGE_WORKFLOW_EDIT. These additions could potentially improve the functionality of systems that work with advanced workflows related to knowledge management.
Here are a few points:
-
Functionality Enhancement: Adding these permissions would allow users with these capabilities to view or edit knowledge workflows, which can be useful in scenarios such as managing processes or approvals associated with knowledge items.
-
Clarity and Organization: Organizing permissions into separate groups like
KNOWLEDGE_,SHARED_KNOWLEDGE_, andRESOURCE_KNOWLDGE_helps in clearly delineating areas where permission sets differ based on data scope (knowledge, shared knowledge, resources). -
Future Proofing: If more complex permission requirements emerge for workflow-related actions, having these distinct sections makes it easier to scale and maintain the system's permission structure.
Overall, the addition of these permissions aligns with best practices for implementing robust access control mechanisms tailored to specific functionalities within an application.
feat: Knowledge workflow permission