Repository navigation
[DSIP-104][ApiServer][UI] Introduce project-scoped authorization - #18718
ruanwenjun wants to merge 27 commits into
Conversation
ec683e6 to
a10d103
Compare
dc7a83e to
f05a11d
Compare
SbloodyS
left a comment
There was a problem hiding this comment.
DEVELOP unexpectedly authorizes project deletion and ownership transfer
Path: dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProjectServiceImpl.java:299,346-361
[Evidence] DSIP-104 defines DEVELOP as creating, editing, and deleting workflow/task definitions. deleteProject nevertheless requires only DEVELOP, so a member granted only that permission can delete an empty project. The update path uses the same permission and then unconditionally changes project.user_id to the editing user.
[Impact] A workflow developer can perform project-lifecycle operations outside the documented permission scope and can silently replace the recorded project owner.
[Request] Define an explicit project-lifecycle policy—system administrator/project owner, or a dedicated permission—and use it for rename/delete. A metadata edit must not change user_id; ownership transfer should be an explicit operation. Add tests for a user holding only DEVELOP.
Schedule validation does not include nested subworkflow resources
Path: dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/SchedulerServiceImpl.java:618-625
[Evidence] Schedule create, update, and online operations call checkScheduleResources, but it validates only the parent workflow's execution settings and direct task definitions. It never calls the recursive TaskSubWorkflowPermissionChecker.
[Impact] If a child workflow's project grants are revoked after that child was published, a new or newly enabled parent schedule still passes validation. Scheduled triggers do not perform API-layer authorization again, so this bypasses the stated “check grants during schedule management” contract.
[Request] Reuse a recursive execution-resource validator from schedule creation/update/online and cover a cross-project child whose datasource or worker-group grant was revoked before the parent schedule is enabled.
Nested subworkflows do not validate inherited execution settings against the child project
Path: dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/permission/TaskSubWorkflowPermissionChecker.java:68-80
[Evidence] The recursive execution check validates only resources embedded in each child task definition. At runtime, SubWorkflowLogicTask copies the parent instance's warning group, worker group, tenant, and environment into every child instance (SubWorkflowLogicTask.java:242-252).
[Impact] A cross-project child can run with a tenant/queue, worker group, environment, or warning group that is granted to the parent project but not to the child project. This contradicts the PR's claim that every child workflow is checked against its own project grants.
[Request] Pass the effective execution settings into recursive validation and call checkExecutionSettings for every child project. Add coverage where child task-level resources are valid but one inherited execution-setting grant is absent.
Several read APIs still trust the caller-supplied project instead of the resource's actual project
Paths:
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/WorkflowInstanceServiceImpl.java:532-552dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/WorkflowInstanceServiceImpl.java:761-773dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/WorkflowLineageServiceImpl.java:96-115
[Evidence] These methods authorize projectCode first, then query a subworkflow instance, trigger relation, or workflow lineage by an independently supplied identifier without proving that the returned object belongs to that project.
[Impact] A user with VIEW on project A can supply an identifier from project B and obtain parent-instance IDs, workflow-instance summaries, or lineage metadata from B.
[Request] Resolve the target instance/workflow first and require VIEW on its actual project, or include project_code in the DAO predicates. Add cross-project negative tests for all three endpoints.
Cross-project workflow move bypasses target resource validation
Path: dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/WorkflowDefinitionServiceImpl.java:1509-1522
[Evidence] moveWorkflowDefinitions changes the workflow's project and calls updateDagDefine with an empty task-definition list. updateDagDefine invokes both resource checkers on that empty list, so they return without inspecting the moved workflow's actual tasks.
[Impact] A workflow referencing datasources, worker groups, environments, namespaces, task groups, or subworkflows can be moved into a project that has none of the required grants. This bypasses the configuration-time authorization contract.
[Request] Load the moved workflow version's real task-definition logs and validate them, including nested subworkflows, against the target project before persisting the move. Add a cross-project move test with one missing target grant.
[Repository requirement] Do not add MyBatis lambda-wrapper persistence
Paths:
dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/repository/impl/ProjectUserPermissionDaoImpl.java:46-82dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/repository/impl/ProjectResourceAuthorizationDaoImpl.java:46-68
[Evidence] Both new repositories implement their queries and deletes through LambdaQueryWrapper.
[Request] Move these operations to explicit Mapper methods and mapper XML, following the repository requirement against MyBatis lambda mapper usage.
[Repository requirement] New services need Service interfaces plus Impl classes
Paths:
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/permission/ProjectAuthorizationService.javadolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/permission/ProjectResourceAuthorizationService.java
[Evidence] Both new Spring services are concrete classes named *Service, with no corresponding interface/*ServiceImpl.
[Request] Preserve the project's Service-plus-Impl ownership convention by introducing interfaces and moving the implementations into impl classes.
SbloodyS
left a comment
There was a problem hiding this comment.
Legacy ownership checks defeat project-scoped delete permissions
Paths:
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/WorkflowDefinitionServiceImpl.java:929-935dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/SchedulerServiceImpl.java:364-378
[Evidence] DSIP-104 and the updated permission guide define DEVELOP as allowing workflow-definition deletion and MANAGE_SCHEDULES as allowing schedule deletion. Both deletion paths check the corresponding project permission, but then retain legacy object-owner checks that reject every non-owner ordinary user.
[Impact] A project member explicitly granted DEVELOP cannot delete a workflow created by another member, and a member granted MANAGE_SCHEDULES cannot delete another member's offline schedule. These project roles therefore cannot perform the operations they promise, leaving shared project administration dependent on the original creator or a system administrator.
[Request] Remove these legacy ownership checks, or document and model ownership as an additional explicit authorization dimension. Add positive tests using non-owner members who hold the required project permissions.
The security model still promises ordinary-user datasource management after making it administrator-only
Paths:
docs/docs/en/contribute/join/security-model.md:44-60dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/permission/ResourcePermissionCheckServiceImpl.java:72-75,121-132,410-421
[Evidence] The security model still states that ordinary users can create datasource connections and modify or delete authorized connections. The implementation now returns no datasource IDs for every non-administrator and makes DataSourceResourcePermissionCheck.permissionCheck always return false. This blocks ordinary-user datasource creation, listing, inspection, update, connection testing, and deletion. The updated tests explicitly assert this administrator-only behavior.
[Impact] The documented security boundary and the shipped behavior disagree. The incompatible-upgrade notes also do not clearly state that ordinary users lose datasource-management access.
[Request] If administrator-only global resource management is intentional, update both language versions of the security model and the 3.5.0 incompatible-change notes explicitly. Otherwise, preserve datasource owner-management permissions while keeping project resource-use grants separate.
Normalize the new K8s namespace mapper lines
Path: dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/K8sNamespaceMapper.xml:79-95
[Evidence] git diff --check apache/dev...pr/18718 reports trailing whitespace on every newly added line in this block, caused by inconsistent line endings.
[Impact] The PR does not currently pass a clean git diff --check, despite the verification notes stating that it does.
[Request] Normalize this block to the repository's line-ending format and rerun git diff --check.
|
Thanks for the review. Addressed all three points in 2aaf1d48f0:
Both new regression tests failed before the fix. After the fix, all 112 focused tests passed, along with Root/API Spotless, documentation image checks and local-link checks. CI for this commit is still queued/running. |
SbloodyS
left a comment
There was a problem hiding this comment.
PostgreSQL upgrades can bypass the default tenant's queue grant
[Fact] checkExecutionSettings skips queue authorization whenever tenant.queueId <= 0. The historical PostgreSQL 3.2.0 upgrade inserts the built-in default tenant with queue_id = 0, while the MySQL upgrade and fresh PostgreSQL schema associate it with the default queue. No later PostgreSQL migration normalizes this value.
[Inference] On an installation upgraded through that PostgreSQL path, granting the default tenant is sufficient to pass execution and schedule checks without granting its queue. Authorization therefore depends on the database type and installation history, contradicting the documented requirement that a configured tenant queue also needs an explicit project grant.
[Request] Normalize the PostgreSQL default tenant's queue during the 3.5.0 upgrade, resolving the actual default queue rather than blindly assuming an ID. Add an upgrade regression starting from the historical PostgreSQL state and assert that execution requires both the default tenant and queue grants.
The timing form does not bind the model required for tenant validation
[Fact] This PR adds a required tenantCode rule, and schedule creation and update both call timingFormRef.validate(). However, the NForm in timing-modal.tsx has no model prop. Naive UI obtains an item's validation value from form.props.model[path], so this rule receives undefined even after the select updates timingForm.tenantCode. The start modal correctly passes its model.
[Inference] The timing modal cannot pass validation, blocking schedule creation and update through the UI for every selected tenant.
[Request] Pass model={this.timingForm} to this NForm and add a browser regression that creates and updates a schedule with an authorized tenant.
Lineage results do not check every related workflow project
[Fact] The new lineage check validates the actual project of the requested root workflow. The method then collects both workflow codes from every lineage record and calls queryWorkFlowLineageByCode for each one without checking the related workflow's project. That query returns workflow names, release states, and schedule timing data. queryWorkFlowLineage and queryDependentWorkflowDefinitions have the same per-related-node gap.
[Inference] If a user originally had VIEW on projects A and B, created a cross-project dependency, and later lost VIEW on B, querying lineage from A still exposes B's workflow and schedule metadata. This contradicts the documented cross-project and revocation semantics.
[Request] Resolve the owning project for every workflow included in a lineage result and enforce VIEW before returning its relations or details. Add a regression that revokes project B visibility after creating an A-to-B lineage.
Resource deletion leaves stale project datasource grants
[Fact] The new datasource permission checker treats rows in t_ds_project_resource_authorization as the complete authorization result and does not verify datasource existence. Datasource deletion removes the datasource and legacy datasource-user rows, but not the new project authorization rows. The new authorization table has no foreign key or cascade that performs this cleanup.
[Inference] A saved or imported task referencing a deleted datasource can still pass save, publish, and execution authorization checks and fail only later when runtime code attempts to load the missing datasource. Project authorization views also retain orphan grant state.
[Request] Revoke all DATASOURCE grants for a deleted datasource in the same transaction. Apply the same lifecycle rule to the other resource types, including name-keyed worker-group updates, and add deletion and rename regressions.
Moving an online workflow bypasses schedule-level grants in the target project
[Fact] The move path checks task resources against the target project and then changes the workflow's projectCode. It does not inspect the associated schedule, require it to be offline, validate its worker group, tenant and queue, environment, or alert group against the target project, or re-register the Quartz job.
[Fact] An existing Quartz job continues loading the same schedule and current workflow definition. WorkflowScheduleTrigger assigns the workflow's new project code to the created instance, while background schedule triggers intentionally do not repeat project resource authorization.
[Inference] Moving an online scheduled workflow lets the target project continue executing with schedule-level resources it was never granted. The move also changes effective scheduled execution without requiring MANAGE_SCHEDULES.
[Request] Reject moves while the workflow or its schedule is online, or atomically take the schedule offline, validate every effective execution setting against the target project, and re-register it under the target project. Add a regression covering an online schedule whose tenant or worker group is not granted to the destination.
|
Thanks for the review. Updated in e51c111da4:
Validation: 211 focused API tests, 14 H2 DAO tests and 2 MySQL/PostgreSQL upgrade tests passed. Chromium checks with mocked APIs reproduced the original form failures and verified the fix, including empty/revoked tenant rejection. Frontend checks/build, Spotless and documentation image checks passed. The new browser E2E test compiles; full API/browser E2E suites were not rerun, and CI for this commit is not yet verified. |
SbloodyS
left a comment
There was a problem hiding this comment.
Workflow version-switch permission boundary
[Fact — paths]
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/WorkflowDefinitionServiceImpl.java:1674-1723dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/process/ProcessServiceImpl.java:520-545docs/docs/en/guide/security/security.md:112-123
[Fact] switchWorkflowDefinitionVersion requires only DEVELOP. ProcessServiceImpl.switchVersion unconditionally changes the selected workflow version to OFFLINE, and the caller then invokes offlineSchedulerByWorkflowCode, which also disables an existing online schedule.
[Fact] DSIP-104 defines DEVELOP, PUBLISH, and MANAGE_SCHEDULES as independent permissions. Taking a workflow offline belongs to PUBLISH, while disabling a schedule belongs to MANAGE_SCHEDULES.
[Inference] A member holding only DEVELOP can switch the version of an online workflow and consequently take both the workflow and its schedule offline. This bypasses two independent permission boundaries and allows a developer to interrupt scheduled production execution.
[Request] Reject workflow version switching while the workflow or its schedule is online, matching the existing task-version behavior, or explicitly require every permission needed by the resulting state changes. Add a regression using a user who has DEVELOP but lacks PUBLISH and MANAGE_SCHEDULES.
Moved-instance project resolution
[Fact — path] dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/WorkflowInstanceServiceImpl.java:299-320,579-615
[Fact] queryTaskListByWorkflowInstanceId and viewVariables load the requested workflow instance, but they do not authorize its persisted workflowInstance.projectCode. Instead, they compare the caller-supplied project with the workflow definition's current project.
[Fact] Moving a workflow creates a new definition version under the destination project, while existing workflow instances retain the source project code.
[Inference] After a move from project A to project B, a user with VIEW only on B can supply an old instance ID from A and pass these checks because the current workflow definition belongs to B. The same instance can become inaccessible to users who still have VIEW on A. This violates DSIP-104's requirement to authorize existing objects through their actual owning project.
[Request] Use workflowInstance.getProjectCode() as the authorization source of truth before reading task-instance or variable data. Do not derive historical instance ownership from the current workflow definition. Add cross-project regressions covering completed instances before and after a workflow move.
Worker-group rename lifecycle
[Fact — path] dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/WorkerGroupServiceImpl.java:143-158
[Fact] Renaming a worker group updates only the worker-group row, revokes every project grant for the old name, and refreshes the Master's worker-group state. It does not call the existing dependency checker and does not update task definitions, schedules, or environment relations that still store the old worker-group name.
[Fact] Granting the new worker-group name cannot repair those existing references because the saved definitions and schedules continue requesting the old name. The old name also cannot be granted again after it no longer resolves to an existing worker group.
[Inference] Renaming a worker group that is referenced by workflows or schedules leaves those objects bound to a resource that no longer exists and no longer has grants. Existing background schedules may continue reaching the trigger path, but task dispatch or subsequent authorization checks will fail on the obsolete name.
[Request] Reuse checkWorkerGroupDependencies to reject renames while the old name is referenced, or atomically migrate all stored references and define an explicit grant-migration policy. Add regressions for task definitions, schedules, and environment relations instead of testing only that the old grants are deleted.
Global resource verification endpoints
[Fact — paths]
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/DataSourceController.java:272-281dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/TenantController.java:198-208dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/QueueController.java:189-201dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/EnvironmentController.java:214-224dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/K8sNamespaceController.java:122-134dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/AlertGroupController.java:222-237
[Fact] These verification endpoints either do not receive the session user or receive it and never pass it to the service. They perform global existence checks without an administrator check or a project resource grant.
[Fact] Other list and detail paths in this PR restrict global resource metadata to administrators and expose ordinary users only to resources granted to a project where they have VIEW.
[Inference] An ordinary authenticated user can enumerate ungranted datasource names, tenant codes, queues, environments, Kubernetes namespaces, and alert-group names by comparing the verification responses. This leaves a metadata side channel outside the newly documented authorization boundary.
[Request] Require system-administrator permission on global verification endpoints. If a verification endpoint is also needed for project-scoped selection, require projectCode, VIEW, and the matching project resource grant rather than exposing the global namespace.
Authorization auditability
[Fact — paths]
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/ProjectAuthorizationController.java:63-69dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/ProjectResourceAuthorizationController.java:55-70dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProjectAuthorizationServiceImpl.java:83-101dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProjectResourceAuthorizationServiceImpl.java:75-96
[Fact] Member-permission replacement and resource grant/revoke operations have no @OperatorLog annotation, corresponding AuditType, or service-level mutation log. Permission replacement first deletes the complete old set and then inserts the new set without recording either state.
[Inference] DolphinScheduler's audit log cannot determine who granted or revoked a project permission or resource, which subject was changed, or what the previous and resulting permission sets were. These are the central security mutations introduced by this PR.
[Request] Add explicit audit events for member-permission replacement and project resource grant/revoke. Record the operator, project code, target user or resource type/key, and before/after permission state without including credentials or sensitive resource configuration. Add audit regressions for successful mutations and ensure failed transactions do not produce successful audit records.
|
Thanks for the review. Updated in 9e9c19bfd0:
Validation: full API verification ran 954 tests with 0 failures/errors and 10 existing skips; 32 H2 DAO tests and 2 MySQL/PostgreSQL upgrade integration tests passed. The latter also exercise the row-lock query on both databases. Frontend checks/build, Spotless, documentation image checks and |
|
Follow-up in a1400b2d4b: failed/suspended recovery was using the parent's recorded task version but then recursively checking the current child definitions. An existing child can still be bound to an older version, so a later child-definition change could incorrectly block recovery. Recovery now retains the parent project's Both recovery regressions reproduced before the fix. All 62 focused tests passed, including revoked parent permission/settings checks and existing rerun/schedule checks. Spotless, documentation image checks and |
SbloodyS
left a comment
There was a problem hiding this comment.
Authorization audit payload exceeds the database schema
[Fact — paths]
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/audit/ProjectAuthorizationAuditLogger.java:42-58dolphinscheduler-dao/src/main/resources/sql/dolphinscheduler_mysql.sql:1208-1221dolphinscheduler-dao/src/main/resources/sql/dolphinscheduler_postgresql.sql:1222-1234dolphinscheduler-dao/src/main/resources/sql/dolphinscheduler_h2.sql:1209-1222
[Fact] ProjectAuthorizationAuditLogger serializes the complete before/after state, action, operator ID/name and project code into t_ds_audit_log.detail. The MySQL schema defines this column as varchar(100). A minimal resource-grant payload with a one-character operator name and resource key is already 154 ASCII characters.
[Fact] PostgreSQL and H2 use varchar(255), but resource keys are allowed to contain up to 255 characters. A valid long worker-group name plus the audit envelope also exceeds those columns. No 3.5.0 migration widens the existing audit column.
[Inference] Under MySQL strict SQL mode, every normal member-permission replacement and resource grant/revoke can fail while inserting its audit row, rolling back the authorization transaction. Without strict mode, MySQL can truncate the JSON, leaving an invalid and unusable audit record. PostgreSQL and H2 remain vulnerable for sufficiently long valid keys or names.
[Request] Widen t_ds_audit_log.detail to a text-capable type in the fresh schemas and database upgrade scripts. Add real MySQL and PostgreSQL authorization-mutation tests that verify both a normal payload and the maximum supported resource-key length, and assert that the stored detail remains parseable JSON.
Recovery can create an unvalidated current subworkflow
[Fact — paths]
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ExecutorServiceImpl.java:279-295dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/command/handler/RecoverFailureTaskCommandHandler.java:53-55,147-204dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/executor/plugin/subworkflow/SubWorkflowLogicTask.java:156-174,192-260
[Fact] Failed and suspended recovery now checks only the parent workflow's pinned tasks and completely skips TaskSubWorkflowPermissionChecker. This is correct only when a subworkflow task already has a bound child instance.
[Fact] RecoverFailureTaskCommandHandler explicitly includes downstream tasks that were never triggered. When such a subworkflow task has no runtime context, SubWorkflowLogicTask calls triggerNewSubWorkflow, loads the child definition's current version, and creates a new child instance from that version.
[Inference] A parent can fail before reaching its subworkflow task, after which the user's child-project EXECUTE permission or the child project's resource grants can be revoked. Recovering the parent still passes the API checks and later creates a new child instance from the current child definition without validating either the child project permission or its resources.
[Request] Distinguish bound child instances from subworkflow tasks that will create a new child. Recover existing children against their persisted instance/version, but retain recursive current-child authorization for unbound or newly recreated subworkflow tasks. Add a regression where the parent fails before the subworkflow starts, the child permission or grant is revoked, and recovery must be rejected before command submission.
The four-permission model no longer implements the linked DSIP
[Fact — paths]
dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/enums/ProjectPermission.java:20-25docs/docs/en/guide/security/security.md:106-125- Linked issue:
apache/dolphinscheduler#17923
[Fact] The current implementation reduces the model to VIEW, DEVELOP, EXECUTE and MANAGE_MEMBERS. Publishing is bundled into DEVELOP, while all schedule management is bundled into EXECUTE.
[Fact] The still-open linked DSIP-104 issue continues to specify six independent permissions: View, Develop, Publish, Execute, Manage schedules and Manage members. It explicitly separates workflow publishing from definition development and schedule management from manual execution. No issue update or maintainer discussion approves replacing that contract with four broader permissions.
[Inference] A user who only needs to develop definitions must also receive publishing authority. A user who only needs to operate schedules must also receive manual run, backfill, rerun, recovery, pause and stop authority, and every ordinary executor automatically gains schedule modification authority. This removes the least-privilege separation that DSIP-104 was designed to introduce.
[Request] Restore the six independent operation permissions defined by the linked DSIP, or obtain explicit design approval and update the DSIP before merging the broader four-permission model. Updating only the PR description, implementation and documentation does not resolve the conflict with the issue this PR closes.
|
Thanks for the review. Updated in 34a4f3b182:
Validation: 41 focused API tests passed, including recovery rejection/success and H2 authorization/audit transaction tests. Two real MySQL/PostgreSQL schema/upgrade tests passed. Spotless, documentation image checks and |
SbloodyS
left a comment
There was a problem hiding this comment.
Additional execution and authorization lifecycle gaps
Recursive execution validation ignores offline child definitions
[Fact — paths]
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/permission/TaskSubWorkflowPermissionChecker.java:68-139dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ExecutorServiceImpl.java:138-190,254-296dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/SchedulerServiceImpl.java:475-493dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/executor/plugin/subworkflow/SubWorkflowLogicTask.java:226-260
[Fact] checkExecutionPermission recursively resolves every current child definition and checks its project EXECUTE permission, resources and inherited execution settings, but it never checks WorkflowDefinition.releaseState.
[Fact] Manual start and backfill validate only the parent definition's online state before calling this recursive checker. Failed/suspended recovery and rerun also use the recursive checker without a child release-state check. When Master creates an unbound child instance, triggerNewSubWorkflow loads the current child definition and submits it without checking whether it is online.
[Fact] Schedule enabling has a separate checkSubWorkflowDefinitionValid call, but that implementation checks only direct children. A direct child that is online but references an offline grandchild passes that check, and the recursive authorization traversal still does not reject the grandchild.
[Inference] After publishing a parent, an administrator can take a referenced child offline. Subsequent manual starts, backfills, reruns or recoveries can still create an instance of that offline child. A newly enabled schedule can likewise reach an offline nested child. This defeats the definition release-state boundary even though project permissions and resource grants pass.
[Request] Extend the execution-time recursive traversal to require every child definition that may create a new instance to be ONLINE. Apply it to manual start, backfill, rerun, recovery and schedule enabling, while keeping pure save or offline schedule-edit paths separate if they are intended to allow offline children. Add regressions for a direct offline child and an online child with an offline grandchild. Existing published background schedules can retain the separately documented no-recheck behavior.
User deletion leaves orphan project permissions
[Fact — paths]
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:414-447dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/repository/ProjectUserPermissionDao.java:25-39dolphinscheduler-dao/src/main/resources/sql/upgrade/3.5.0_schema/mysql/dolphinscheduler_ddl.sql:18-27dolphinscheduler-dao/src/main/resources/sql/upgrade/3.5.0_schema/postgresql/dolphinscheduler_ddl.sql:18-27
[Fact] Deleting a non-owner user removes access tokens, sessions and the user row, but does not remove rows from t_ds_project_user_permission. The new permission table has no foreign key or delete cascade, and its DAO has no revoke-by-user operation.
[Inference] Every deleted project member leaves hidden authorization rows indefinitely. If a user ID is restored or reused through import, database repair or sequence manipulation, the replacement user inherits the deleted account's project permissions. Even without ID reuse, authorization-level queries and database maintenance retain state for a subject that no longer exists.
[Request] Add an explicit Mapper/XML revokeAllPermissionsByUserId operation and invoke it in the same transaction before deleting the user. Add a regression that grants a non-owner permissions in multiple projects, deletes the user, and verifies that all new-table grants are removed while a failed user deletion rolls the cleanup back.
|
Thanks for the review. Both findings are addressed in 992e5c2093:
Merged the current Validation after the merge: full API verification ran 976 tests with 0 failures/errors and 10 skips; 14 DAO tests and both real MySQL/PostgreSQL schema/upgrade tests passed. Spotless, documentation image/link checks and the cumulative diff check passed. Browser E2E was not rerun locally. The previous head's dependency scan failed its vulnerability threshold, and Shell E2E reported a Chrome renderer timeout. New-head CI is not yet verified. |
| } | ||
| if ("K8S".equals(task.getTaskType()) || "KUBEFLOW".equals(task.getTaskType()) | ||
| || "SPARK".equals(task.getTaskType())) { | ||
| String namespace = JSONUtils.toMap(task.getTaskParams()).get("namespace"); |
There was a problem hiding this comment.
Saving a Spark task with mainJar fails before namespace authorization
Path: dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/permission/TaskResourcePermissionChecker.java:83-85
checkPermission parses the whole taskParams with JSONUtils.toMap, which targets Map<String, String>. With Jackson 2.13.4, numbers, booleans, nulls and empty arrays are accepted. Nested objects are rejected.
This is deterministic for the default Spark form. programType defaults to SCALA, and use-main-jar.ts requires mainJar unless the type is SQL. format-data.ts writes the selection as mainJar: { resourceName }. Saving goes through WorkflowDefinitionServiceImpl.createDagDefine → checkPermission. PYTHON has the same requirement. A SQL Spark task without nested fields does not hit this.
Payload:
{"mainClass":"org.apache.spark.examples.SparkPi","mainJar":{"resourceName":"spark-examples.jar"},"programType":"SCALA","driverCores":1,"localParams":[],"resourceList":[]}JSONUtils.toMap fails at mainJar on every save:
MismatchedInputException: Cannot deserialize value of type `java.lang.String` from Object value (token `JsonToken.START_OBJECT`)
(through reference chain: java.util.LinkedHashMap["mainJar"])
The API returns 500 and the workflow is not saved. Namespace can be empty. This happens before the worker-group grant check. The same toMap(taskParams) also fails when localParams or resourceList contains an object, or when a K8S task has a non-empty customizedLabels array.
namespaceGrantMustMatchBothNameAndCluster only uses {"namespace":"{\"name\":\"jobs\",\"cluster\":\"production\"}"}, so it does not catch this.
Please read namespace with JsonNode or JSONUtils.getNodeString, and keep the existing JSONUtils.toMap(namespace) for the {"name","cluster"} string. Spark, K8S and Kubeflow already share that value shape (useNamespace(), and SparkTask reads toMap(namespace).get("name")). Add a regression whose taskParams includes mainJar.
There was a problem hiding this comment.
Thanks for catching this. Fixed in 680f586c66.
The checker now reads only the top-level namespace with JsonNode, preserving the existing namespace JSON-string parsing and name/cluster grant checks. Non-string namespace values are rejected.
Regression coverage includes Scala/Python mainJar, non-empty localParams, resourceList and customizedLabels, and missing/null namespaces. The tests also verify that worker-group grants, namespace grants and cluster matching are still enforced.
All 165 focused API tests passed, along with Spotless and git diff --check. Full API/browser E2E suites were not rerun for this commit; new-head CI is not yet verified.
SbloodyS
left a comment
There was a problem hiding this comment.
Task-level YARN queues bypass project QUEUE grants
[Fact] TaskResourcePermissionChecker.checkPermission collects and validates datasources, worker groups, environments, task groups, SQL alert groups, and Kubernetes namespaces, but it never reads the task-level yarnQueue. checkExecutionSettings validates only the queue associated with the selected tenant.
[Fact] The UI serializes yarnQueue into the task parameters for Spark, MapReduce, Flink, and Flink Stream tasks. The corresponding runtime implementations then pass this value directly to Spark's --queue, MapReduce's mapreduce.job.queuename, or Flink's YARN queue options.
[Inference] A project can therefore hold a grant for the tenant's queue while saving and executing a task whose yarnQueue names a different, ungranted queue. Both definition-time and execution-time authorization pass because they call TaskResourcePermissionChecker, while the Worker later submits the job to the task-level queue. This contradicts the documented rule that task-referenced resources must be granted to the project.
[Fact] The raw others field can also contain the same queue options, and the Spark, MapReduce, and Flink command builders prefer those options over the structured yarnQueue value. Validating only the structured field would therefore leave another built-in path around the grant.
[Inference] Please define and enforce one consistent boundary. If project queue grants are intended to cover task-level YARN queues, resolve the structured yarnQueue against t_ds_queue.queue, require the matching queue ID grant during save, online, schedule validation, and manual execution, and handle the recognized queue options in others consistently. If grants intentionally cover only the tenant's queue, narrow the documentation and UI wording so they do not promise authorization for all task-referenced queues. Add regressions for Spark, MapReduce, Flink, and Flink Stream using a granted tenant queue together with an ungranted task-level queue.
Was this PR generated or assisted by AI?
YES. The implementation, UI, schema upgrades, tests and documentation were generated or assisted by AI.
Purpose of the pull request
Implements DSIP-104 by separating project operation permissions from shared-resource grants. Users receive permissions within a project; resources are granted to projects. An operation requires both the relevant project permission and the resource grants it uses.
Closes #17923.
Authorization model
Users receive four project operation permissions:
VIEW,DEVELOP,EXECUTEandMANAGE_MEMBERS.DEVELOPincludes definition publishing;EXECUTEincludes schedule management. These are permissions assigned directly to users, not separate role entities. Supported resources are datasources, worker groups, tenants, queues, environments, alert groups, Kubernetes namespaces and task groups. Resource files retain their existing authorization mechanism.System administrators grant or revoke project resources. System administrators and users with
MANAGE_MEMBERSmanage member permissions within the relevant project. Resource-use grants do not confer resource modification, deletion or re-granting rights. System administrators pass the project operation check, but required resource grants still apply.Project metadata updates and deletion are restricted to system administrators or project creators.
DEVELOPcovers workflow/task definitions and their publishing; editing project metadata preserves its recorded owner.Before DSIP-104
Datasource and namespace grants belong to users. Worker groups already have a separate project association; other resources follow their existing ownership or resource-specific rules.
After DSIP-104
All eight resource-use grants belong to the project. Each operation checks the user's permission in that project and the required project grants.
Brief change log
VIEW,DEVELOP(including publishing),EXECUTE(including schedule management), andMANAGE_MEMBERS. Align backend checks, compatibility queries, UI labels, both language guides, upgrade notes and the authorization diagram.DEVELOPandEXECUTErespectively, subject to existing state and dependency constraints. Add non-owner deletion regressions and update obsolete ownership assertions.K8sNamespaceMapper.xmlto LF without changing its SQL and validate whitespace across the entire PR diff.defaulttenant (id=-1). Reuse its existing database record; no additional tenant insert is required. Startup and schedule forms use authorized tenant choices, require a selection when needed and clear revoked choices. Workflow E2E fixtures explicitly select their granted tenant.api.service.impl.Upgrade: Legacy authorization tables and data are retained, without automatic permission migration or fallback. Administrators must explicitly assign project member permissions and project resource grants, including any required default resources.
Revocation: Subsequent manual operations and schedule creation, update or enabling validate current grants. Existing running/queued instances, published schedule triggers and automatic retries continue without rechecking project grants. Revocation does not stop those instances or disable existing schedules.
Verify this pull request
Latest recovery-fix verification at
a1400b2d4b:git diff --checkpassed. Both language guides distinguish recovery from rerunning; Master recovery behavior is unchanged.Earlier review-fix verification at
9e9c19bfd0:git diff --checkpassed. The draw.io-rendered diagram shows four permissions; no.drawiofiles are committed.Earlier review-fix verification at
e51c111da4:queue_idremains0) and passes with the actual default queue at ID42; reapplying the migration preserves an explicitly configured queue.git diff --checkpassed.Earlier review-fix verification at
2aaf1d48f0:git diff --check upstream/dev...HEADpassed for the entire cumulative PR diff. The namespace mapper parses as XML and is byte-equivalent to its preceding content after normalizing CRLF to LF.Documentation verification at
514a283cba:git diff --checkpassed.Code verification at
0332c02a3b:git diff --checkpassed. Execution-time authorization semantics are unchanged.Full API verification at
e86016e109:git diff --checkpassed.Other validation:
8130a4d927. CodeQL also passed ate86016e109. These CI results apply to the listed earlier commits.34cee246f6, including explicit default-tenant/queue grants and rejection before a required worker-group grant.2eca6ace52, H2 DAO verification (13 passed), shared startup/schedule tenant-selection checks (14 scenarios), frontend TypeScript/ESLint/Prettier/production build and documentation image checks passed. API-test and E2E sources compiled on Java 11.Pull Request Notice
Pull Request Notice