mirror of
https://github.com/langgenius/dify.git
synced 2026-08-31 01:36:38 +08:00
fix(api): allow external knowledge base creation under enterprise RBAC (#39930)
This commit is contained in:
@@ -353,7 +353,9 @@ class ExternalDatasetCreateApi(Resource):
|
||||
@login_required
|
||||
@account_initialization_required
|
||||
@edit_permission_required
|
||||
@rbac_permission_required(RBACResourceScope.DATASET, RBACPermission.DATASET_EXTERNAL_CONNECT)
|
||||
@rbac_permission_required(
|
||||
RBACResourceScope.DATASET, RBACPermission.DATASET_EXTERNAL_CONNECT, resource_required=False
|
||||
)
|
||||
@with_current_user
|
||||
@with_current_tenant_id
|
||||
@with_session
|
||||
|
||||
@@ -19,6 +19,7 @@ from models.dataset import (
|
||||
ExternalKnowledgeApis,
|
||||
ExternalKnowledgeBindings,
|
||||
)
|
||||
from services.enterprise import rbac_service as enterprise_rbac_service
|
||||
from services.entities.external_knowledge_entities.external_knowledge_entities import (
|
||||
Authorization,
|
||||
ExternalKnowledgeApiSetting,
|
||||
@@ -331,6 +332,12 @@ class ExternalDatasetService:
|
||||
session.add(external_knowledge_binding)
|
||||
|
||||
session.commit()
|
||||
enterprise_rbac_service.try_sync_creator_access_policy_member_bindings(
|
||||
tenant_id,
|
||||
user_id,
|
||||
enterprise_rbac_service.RBACResourceType.DATASET,
|
||||
dataset.id,
|
||||
)
|
||||
|
||||
return dataset
|
||||
|
||||
|
||||
@@ -0,0 +1,102 @@
|
||||
"""Guard against resource-scoped RBAC gates mounted on routes that carry no resource id.
|
||||
|
||||
``rbac_permission_required`` defaults to ``resource_required=True``, which makes
|
||||
``_extract_resource_id`` raise ``ValueError`` when the matched path holds none of the
|
||||
accepted identifiers. The request then fails with a 400 before the view ever runs, so the
|
||||
endpoint is unreachable for every tenant with ``RBAC_ENABLED``. Creation endpoints and
|
||||
other workspace-level actions must opt out with ``resource_required=False``.
|
||||
"""
|
||||
|
||||
import ast
|
||||
from pathlib import Path
|
||||
|
||||
CONTROLLERS_DIR = Path(__file__).resolve().parents[3] / "controllers"
|
||||
|
||||
# Mirrors the lookup order in controllers/common/wraps.py::_extract_resource_id.
|
||||
ACCEPTED_PATH_ARGS = {
|
||||
"APP": ("app_id", "agent_id", "resource_id"),
|
||||
"DATASET": ("dataset_id", "pipeline_id", "resource_id"),
|
||||
}
|
||||
|
||||
# Known violations tracked separately: DatasetDocumentSegmentBatchImportApi binds one class
|
||||
# to both the dataset-scoped import route and the job-scoped status route, so every method
|
||||
# on it is reachable at a path carrying only a job id. Its permission points are genuinely
|
||||
# per-dataset, so it needs the route split rather than resource_required=False. Remove
|
||||
# these entries with that fix.
|
||||
KNOWN_VIOLATIONS = {
|
||||
("console/datasets/datasets_segments.py", "DatasetDocumentSegmentBatchImportApi", "post"),
|
||||
("console/datasets/datasets_segments.py", "DatasetDocumentSegmentBatchImportApi", "get"),
|
||||
}
|
||||
|
||||
|
||||
def _decorator_name(node: ast.Call) -> str:
|
||||
func = node.func
|
||||
parts = []
|
||||
while isinstance(func, ast.Attribute):
|
||||
parts.append(func.attr)
|
||||
func = func.value
|
||||
if isinstance(func, ast.Name):
|
||||
parts.append(func.id)
|
||||
return ".".join(reversed(parts))
|
||||
|
||||
|
||||
def _attribute_name(node: ast.expr | None) -> str | None:
|
||||
return node.attr if isinstance(node, ast.Attribute) else None
|
||||
|
||||
|
||||
def _route_paths(class_node: ast.ClassDef) -> list[str]:
|
||||
paths: list[str] = []
|
||||
for decorator in class_node.decorator_list:
|
||||
if isinstance(decorator, ast.Call) and _decorator_name(decorator).endswith(".route"):
|
||||
paths.extend(
|
||||
arg.value for arg in decorator.args if isinstance(arg, ast.Constant) and isinstance(arg.value, str)
|
||||
)
|
||||
return paths
|
||||
|
||||
|
||||
def _path_args(route: str) -> set[str]:
|
||||
args = set()
|
||||
args.update(segment.split(">")[0].split(":")[-1] for segment in route.split("<")[1:])
|
||||
return args
|
||||
|
||||
|
||||
def _resource_scoped_gates(method: ast.FunctionDef | ast.AsyncFunctionDef) -> list[str]:
|
||||
scopes = []
|
||||
for decorator in method.decorator_list:
|
||||
if not (isinstance(decorator, ast.Call) and _decorator_name(decorator).endswith("rbac_permission_required")):
|
||||
continue
|
||||
keywords = {keyword.arg: keyword.value for keyword in decorator.keywords}
|
||||
resource_required = keywords.get("resource_required")
|
||||
if isinstance(resource_required, ast.Constant) and resource_required.value is False:
|
||||
continue
|
||||
scope = _attribute_name(decorator.args[0] if decorator.args else keywords.get("resource_type"))
|
||||
if scope in ACCEPTED_PATH_ARGS:
|
||||
scopes.append(scope)
|
||||
return scopes
|
||||
|
||||
|
||||
def test_resource_scoped_rbac_gates_have_a_resource_id_in_the_route() -> None:
|
||||
violations = []
|
||||
|
||||
for path in sorted(CONTROLLERS_DIR.rglob("*.py")):
|
||||
tree = ast.parse(path.read_text(encoding="utf-8"))
|
||||
for class_node in (node for node in ast.walk(tree) if isinstance(node, ast.ClassDef)):
|
||||
routes = _route_paths(class_node)
|
||||
if not routes:
|
||||
continue
|
||||
methods = (node for node in class_node.body if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef))
|
||||
for method in methods:
|
||||
for scope in _resource_scoped_gates(method):
|
||||
accepted = set(ACCEPTED_PATH_ARGS[scope])
|
||||
unscoped = [route for route in routes if not _path_args(route) & accepted]
|
||||
if not unscoped:
|
||||
continue
|
||||
key = (path.relative_to(CONTROLLERS_DIR).as_posix(), class_node.name, method.name)
|
||||
if key in KNOWN_VIOLATIONS:
|
||||
continue
|
||||
violations.append(f"{key[0]}::{key[1]}.{key[2]} scope={scope} routes={unscoped}")
|
||||
|
||||
assert not violations, (
|
||||
"resource-scoped rbac_permission_required on routes without a resource id; "
|
||||
"pass resource_required=False for workspace-level actions:\n" + "\n".join(violations)
|
||||
)
|
||||
@@ -15,6 +15,7 @@ import pytest
|
||||
|
||||
from constants import HIDDEN_VALUE
|
||||
from models.dataset import Dataset, ExternalKnowledgeApis, ExternalKnowledgeBindings
|
||||
from services.enterprise.rbac_service import RBACResourceType
|
||||
from services.entities.external_knowledge_entities.external_knowledge_entities import (
|
||||
Authorization,
|
||||
AuthorizationConfig,
|
||||
@@ -1614,6 +1615,41 @@ class TestExternalDatasetServiceCreateDataset:
|
||||
mock_db.session.flush.assert_called_once()
|
||||
mock_db.session.commit.assert_called_once()
|
||||
|
||||
@patch("services.external_knowledge_service.enterprise_rbac_service.try_sync_creator_access_policy_member_bindings")
|
||||
@patch("services.external_knowledge_service.db")
|
||||
def test_create_external_dataset_syncs_creator_access_policy_binding(
|
||||
self, mock_db, mock_sync, factory: ExternalDatasetServiceTestDataFactory
|
||||
):
|
||||
"""The creator must be bound to the new dataset's access policy, as in create_empty_dataset."""
|
||||
|
||||
# Arrange
|
||||
def assign_dataset_id(instance):
|
||||
# The real INSERT populates the primary key; a mocked session never flushes.
|
||||
if isinstance(instance, Dataset):
|
||||
instance.id = "dataset-705"
|
||||
|
||||
args = {
|
||||
"name": "Bound External Dataset",
|
||||
"external_knowledge_api_id": "api-703",
|
||||
"external_knowledge_id": "knowledge-704",
|
||||
}
|
||||
mock_db.session.scalar.side_effect = [None, factory.create_external_knowledge_api_mock(api_id="api-703")]
|
||||
mock_db.session.add.side_effect = assign_dataset_id
|
||||
|
||||
# Act
|
||||
dataset = ExternalDatasetService.create_external_dataset(
|
||||
"tenant-701", "user-702", args, session=mock_db.session
|
||||
)
|
||||
|
||||
# Assert
|
||||
assert dataset.id == "dataset-705"
|
||||
mock_sync.assert_called_once_with(
|
||||
"tenant-701",
|
||||
"user-702",
|
||||
RBACResourceType.DATASET,
|
||||
"dataset-705",
|
||||
)
|
||||
|
||||
@patch("services.external_knowledge_service.db")
|
||||
def test_create_external_dataset_duplicate_name_error(
|
||||
self, mock_db, factory: ExternalDatasetServiceTestDataFactory
|
||||
|
||||
Reference in New Issue
Block a user