qiuyanjun888 commented on issue #17923:
URL:
https://github.com/apache/dolphinscheduler/issues/17923#issuecomment-4888555088
Hi @ruanwenjun, I looked through this DSIP-104 issue and the current
permission-checking code. I think this should be solved as a long-term
authorization model refactor rather than only replacing `Object[]` with
generics.
## Current code observations
From the current API-server code, DolphinScheduler does not yet have a
complete RBAC model. Permission checks are currently a mixture of:
- `user_type` admin / normal-user checks
- owner checks
- relation tables such as `t_ds_relation_project_user`,
`t_ds_relation_datasource_user`, `t_ds_relation_resources_user`,
`t_ds_relation_udfs_user`, `t_ds_relation_namespace_user`
- `perm` bit masks such as read / write / execute / all
- `AuthorizationType`
- string API function keys from `ApiFuncIdentificationConstant`
The two existing helpers also have different limitations:
1. `PermissionCheck` is legacy and limited. For example, some callers pass
resource-file authorization types, but the underlying unauthorized-resource
listing logic is not consistently implemented for all resource types.
2. `ResourcePermissionCheckService` centralizes more checks, but its core
API is weakly typed:
- `Object authorizationType`
- `Object[] needChecks`
- `Set<Object>`
This makes it easy to pass a wrong resource id type or wrong resource kind
without compile-time protection. Also, `permissionKey` is currently not a real
RBAC permission in many checkers; several implementations hard-code operation
permission as `true` or `false`.
## Proposed long-term design
I suggest introducing a unified authorization model with two explicit layers:
```text
Authorization decision = admin/super-admin bypass
OR RBAC operation permission
AND resource-instance permission / ownership
```
Pure RBAC is not enough for DolphinScheduler, because many operations must
be authorized against a specific project, datasource, resource file, UDF,
workflow, etc. So the stable model should combine **RBAC operation
permissions** with **resource-instance ACL / owner permissions**.
### 1. Strong permission concepts
Introduce strong types instead of passing raw `Object` values:
- `ResourceType`
- examples: `PROJECT`, `DATASOURCE`, `RESOURCE_FILE`, `UDF`,
`WORKFLOW_DEFINITION`, `TASK_DEFINITION`, `TENANT`, `QUEUE`, `WORKER_GROUP`,
`ENVIRONMENT`, `ALERT_GROUP`, `ALERT_PLUGIN_INSTANCE`, `ACCESS_TOKEN`, `USER`,
`CLUSTER`
- `Operation`
- examples: `CREATE`, `READ`, `UPDATE`, `DELETE`, `EXECUTE`, `USE`,
`GRANT`, `REVOKE`, `LIST`, `IMPORT`, `EXPORT`, `ONLINE`, `OFFLINE`
- `Permission`
- logically `(resourceType, operation)`
- the string key can still exist as the storage / serialization form, but
the code path should use strong types
- `ResourceRef`
- typed resource references such as `ProjectRef.byCode(projectCode)`,
`ProjectRef.byId(projectId)`, `DataSourceRef.byId(datasourceId)`,
`WorkerGroupRef.byName(workerGroup)`, `ResourceFileRef.byId(resourceId)`
This removes the need for `Object[]` and makes resource kind + identifier
type explicit.
### 2. Central `AuthorizationService`
Add a single authorization entry point, for example:
```java
AuthorizationDecision check(AuthorizationRequest request);
AuthorizationRequest.builder()
.user(loginUser)
.resourceType(ResourceType.PROJECT)
.operation(Operation.UPDATE)
.resource(ProjectRef.byCode(projectCode))
.build();
```
`AuthorizationDecision` should carry enough information for consistent error
handling and diagnostics:
- allowed / denied
- reason or status code
- missing permission
- resource type
- operation
- resource identity
For convenience, it can also expose typed helper methods:
```java
checkProject(user, Operation.UPDATE, projectCode);
checkDataSource(user, Operation.USE, datasourceId);
checkGlobal(user, ResourceType.WORKER_GROUP, Operation.CREATE);
```
### 3. RBAC operation-permission tables
Add a formal RBAC layer:
```text
t_ds_role
t_ds_permission
t_ds_role_permission
t_ds_user_role
```
Suggested semantics:
- `t_ds_permission`: built-in permission definitions, e.g. `(PROJECT,
CREATE)`, `(PROJECT, UPDATE)`, `(DATASOURCE, USE)`, `(WORKER_GROUP, CREATE)`
- `t_ds_role_permission`: role -> permission mapping
- `t_ds_user_role`: user -> role mapping
- built-in roles such as `ADMIN` and `GENERAL_USER`
Migration compatibility:
- users with `user_type = ADMIN_USER` should be mapped to the built-in
`ADMIN` role
- ordinary users should be mapped to a default/general role
- `user_type` can be kept for compatibility during migration, but new
permission checks should not keep spreading direct `user_type` logic
### 4. Resource-instance ACL / owner layer
Keep explicit instance-level authorization because DolphinScheduler already
has many user-resource relation tables.
A long-term unified table could be:
```text
t_ds_resource_acl
- id
- subject_type -- USER / ROLE
- subject_id
- resource_type
- resource_id
- permission_mask -- READ / WRITE / EXECUTE / GRANT, etc.
- create_time
- update_time
```
Existing relation tables can be migrated into this model:
- `t_ds_relation_project_user` -> `PROJECT` ACL
- `t_ds_relation_datasource_user` -> `DATASOURCE` ACL
- `t_ds_relation_resources_user` -> `RESOURCE_FILE` ACL
- `t_ds_relation_udfs_user` -> `UDF` ACL
- `t_ds_relation_namespace_user` -> `K8S_NAMESPACE` ACL
The old relation tables should not be removed immediately. A safer migration
path is:
1. add the new authorization model and ACL table
2. migrate existing data into the new ACL table
3. keep compatibility/adapters for one transition period
4. replace call sites gradually
5. remove legacy relation-specific permission readers in a later version
### 5. Resource-specific resolvers
Split resource-specific logic out of `ResourcePermissionCheckServiceImpl`
into focused resolvers:
```java
interface ResourcePermissionResolver<T extends ResourceRef> {
ResourceType resourceType();
boolean exists(T resourceRef);
Optional<Integer> ownerUserId(T resourceRef);
boolean hasAcl(User user, T resourceRef, Operation operation);
}
```
Examples:
- `ProjectPermissionResolver`
- `DataSourcePermissionResolver`
- `ResourceFilePermissionResolver`
- `UdfPermissionResolver`
- `WorkerGroupPermissionResolver`
- `EnvironmentPermissionResolver`
This makes each resource authorization rule testable and avoids continuing
to grow a large checker map based on weak `Object` parameters.
### 6. Invocation style
I would keep explicit service-layer checks as the main mechanism because
many checks require business context, such as resolving `projectCode`, parsing
task resource ids, or checking related resource ownership.
Annotations can still be added later for simple global permissions, for
example:
```java
@RequiresPermission(resource = ResourceType.WORKER_GROUP, operation =
Operation.CREATE)
```
But annotation-only authorization is probably not sufficient for the API
server because many permissions are instance-specific.
### 7. Migration from existing helpers
Suggested migration sequence under the same final architecture:
1. Introduce `ResourceType`, `Operation`, `ResourceRef`,
`AuthorizationRequest`, `AuthorizationDecision`, and `AuthorizationService`.
2. Add RBAC and ACL persistence models.
3. Map current users and relation-table data into the new model.
4. Internally adapt `ResourcePermissionCheckService` to delegate to
`AuthorizationService`.
5. Replace `canOperatorPermissions(...)` call sites with typed authorization
calls.
6. Remove or deprecate `PermissionCheck`.
7. Remove the weak `Object`-based permission APIs after call sites are
migrated.
### 8. Test coverage
The refactor should include tests for:
- admin bypass
- normal user without permission denied
- user role has / misses operation permission
- owner allowed
- ACL read allowed but update/delete denied
- ACL write / execute allowed
- multiple resource refs where one is unauthorized
- resource not found
- migration from existing relation tables and `perm` masks
- service-level regressions for project, datasource, resource file/UDF,
worker group, tenant, environment, alert group/plugin, access token, and
task-definition release with resource ids
## Question
@ruanwenjun does this direction match the intended DSIP-104 scope? In
particular, I think the important design point is that the new model should not
be pure RBAC only; it should be **RBAC for operation permissions +
resource-instance ACL/owner checks** so that DolphinScheduler keeps its
existing project/datasource/resource-level authorization semantics while
getting a typed and maintainable permission API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
