fix: enforce merger role boundaries (Closes #539) #596
+63
-6
@@ -211,6 +211,21 @@ def _effective_workspace_role() -> str:
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _profile_role_kind(profile: dict) -> str:
|
||||||
|
"""Resolve a profile's declared role before inferring from permissions."""
|
||||||
|
role = (profile.get("role") or profile.get("role_kind") or "").strip()
|
||||||
|
if role:
|
||||||
|
return role
|
||||||
|
profile_name = (profile.get("profile_name") or "").strip().lower()
|
||||||
|
for candidate in ("reconciler", "merger", "reviewer", "author"):
|
||||||
|
if candidate in profile_name:
|
||||||
|
return candidate
|
||||||
|
return _role_kind(
|
||||||
|
profile.get("allowed_operations") or [],
|
||||||
|
profile.get("forbidden_operations") or [],
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
def _actual_profile_role() -> str:
|
def _actual_profile_role() -> str:
|
||||||
"""Resolve the workspace role from the ACTIVE PROFILE alone (#540).
|
"""Resolve the workspace role from the ACTIVE PROFILE alone (#540).
|
||||||
|
|
||||||
@@ -223,10 +238,7 @@ def _actual_profile_role() -> str:
|
|||||||
``author`` here, so author blocking is preserved.
|
``author`` here, so author blocking is preserved.
|
||||||
"""
|
"""
|
||||||
profile = get_profile()
|
profile = get_profile()
|
||||||
role = _role_kind(
|
role = _profile_role_kind(profile)
|
||||||
profile.get("allowed_operations") or [],
|
|
||||||
profile.get("forbidden_operations") or [],
|
|
||||||
)
|
|
||||||
return nwb.normalize_role_kind(
|
return nwb.normalize_role_kind(
|
||||||
role,
|
role,
|
||||||
profile_name=profile.get("profile_name"),
|
profile_name=profile.get("profile_name"),
|
||||||
@@ -9917,6 +9929,24 @@ def gitea_resolve_task_capability(
|
|||||||
|
|
||||||
required_permission = task_capability_map.required_permission(task)
|
required_permission = task_capability_map.required_permission(task)
|
||||||
required_role = task_capability_map.required_role(task)
|
required_role = task_capability_map.required_role(task)
|
||||||
|
role_exclusive_tasks = {
|
||||||
|
"review_pr",
|
||||||
|
"approve_pr",
|
||||||
|
"request_changes_pr",
|
||||||
|
"blind_pr_queue_review",
|
||||||
|
"pr_queue_cleanup",
|
||||||
|
"pr-queue-cleanup",
|
||||||
|
"merge_pr",
|
||||||
|
"create_branch",
|
||||||
|
"push_branch",
|
||||||
|
"create_pr",
|
||||||
|
"commit_files",
|
||||||
|
"gitea_commit_files",
|
||||||
|
"address_pr_change_requests",
|
||||||
|
"delete_branch",
|
||||||
|
"work_issue",
|
||||||
|
"work-issue",
|
||||||
|
}
|
||||||
|
|
||||||
infra_assessment = role_session_router.assess_infra_stop(PROJECT_ROOT)
|
infra_assessment = role_session_router.assess_infra_stop(PROJECT_ROOT)
|
||||||
if required_role == "reviewer":
|
if required_role == "reviewer":
|
||||||
@@ -10011,11 +10041,26 @@ def gitea_resolve_task_capability(
|
|||||||
# Load active permissions
|
# Load active permissions
|
||||||
active_allowed = profile.get("allowed_operations") or []
|
active_allowed = profile.get("allowed_operations") or []
|
||||||
active_forbidden = profile.get("forbidden_operations") or []
|
active_forbidden = profile.get("forbidden_operations") or []
|
||||||
|
active_role_kind = _profile_role_kind(profile)
|
||||||
|
|
||||||
# Check if allowed in current session
|
# Check if allowed in current session
|
||||||
allowed_in_current_session, _ = gitea_config.check_operation(
|
permission_allowed_in_current_session, _ = gitea_config.check_operation(
|
||||||
required_permission, active_allowed, active_forbidden
|
required_permission, active_allowed, active_forbidden
|
||||||
)
|
)
|
||||||
|
role_matches_current_session = (
|
||||||
|
task not in role_exclusive_tasks
|
||||||
|
or active_role_kind == required_role
|
||||||
|
)
|
||||||
|
role_mismatch_reason = None
|
||||||
|
if not role_matches_current_session:
|
||||||
|
role_mismatch_reason = (
|
||||||
|
f"Active profile role '{active_role_kind}' cannot perform "
|
||||||
|
f"{required_role} task '{task}' even if nearby permissions are "
|
||||||
|
"present (fail closed)."
|
||||||
|
)
|
||||||
|
allowed_in_current_session = (
|
||||||
|
permission_allowed_in_current_session and role_matches_current_session
|
||||||
|
)
|
||||||
|
|
||||||
switching = gitea_config.is_runtime_switching_enabled()
|
switching = gitea_config.is_runtime_switching_enabled()
|
||||||
available_in_session = allowed_in_current_session
|
available_in_session = allowed_in_current_session
|
||||||
@@ -10042,7 +10087,13 @@ def gitea_resolve_task_capability(
|
|||||||
except Exception:
|
except Exception:
|
||||||
pass
|
pass
|
||||||
ok, _ = gitea_config.check_operation(required_permission, p_allowed_n, p_forbidden_n)
|
ok, _ = gitea_config.check_operation(required_permission, p_allowed_n, p_forbidden_n)
|
||||||
if ok:
|
p_role = (p_data.get("role") or "").strip()
|
||||||
|
p_role_ok = (
|
||||||
|
task not in role_exclusive_tasks
|
||||||
|
or not p_role
|
||||||
|
or p_role == required_role
|
||||||
|
)
|
||||||
|
if ok and p_role_ok:
|
||||||
matching_profiles.append(p_name)
|
matching_profiles.append(p_name)
|
||||||
|
|
||||||
configured = len(matching_profiles) > 0
|
configured = len(matching_profiles) > 0
|
||||||
@@ -10070,6 +10121,8 @@ def gitea_resolve_task_capability(
|
|||||||
reason_msg = (
|
reason_msg = (
|
||||||
f"No profile configured with permission '{required_permission}'."
|
f"No profile configured with permission '{required_permission}'."
|
||||||
)
|
)
|
||||||
|
elif role_mismatch_reason:
|
||||||
|
reason_msg = role_mismatch_reason
|
||||||
different_namespace_required = False
|
different_namespace_required = False
|
||||||
next_safe_action = "None; ready for operations."
|
next_safe_action = "None; ready for operations."
|
||||||
|
|
||||||
@@ -10101,6 +10154,8 @@ def gitea_resolve_task_capability(
|
|||||||
# into author-side mutations.
|
# into author-side mutations.
|
||||||
stop_required = not allowed_in_current_session
|
stop_required = not allowed_in_current_session
|
||||||
task_role_guidance = []
|
task_role_guidance = []
|
||||||
|
if role_mismatch_reason:
|
||||||
|
task_role_guidance.append(f"STOP: {role_mismatch_reason}")
|
||||||
if required_role == "reviewer":
|
if required_role == "reviewer":
|
||||||
if allowed_in_current_session:
|
if allowed_in_current_session:
|
||||||
task_role_guidance.append(
|
task_role_guidance.append(
|
||||||
@@ -10146,7 +10201,9 @@ def gitea_resolve_task_capability(
|
|||||||
"required_role_kind": required_role,
|
"required_role_kind": required_role,
|
||||||
"active_profile": profile["profile_name"],
|
"active_profile": profile["profile_name"],
|
||||||
"active_identity": username,
|
"active_identity": username,
|
||||||
|
"active_role_kind": active_role_kind,
|
||||||
"active_profile_allowed_operations": active_allowed,
|
"active_profile_allowed_operations": active_allowed,
|
||||||
|
"active_profile_permission_allowed": permission_allowed_in_current_session,
|
||||||
"allowed_in_current_session": allowed_in_current_session,
|
"allowed_in_current_session": allowed_in_current_session,
|
||||||
"available_in_session": available_in_session,
|
"available_in_session": available_in_session,
|
||||||
"configured": configured,
|
"configured": configured,
|
||||||
|
|||||||
+28
-2
@@ -55,7 +55,6 @@ def skip_python_scan_walk_root(project_root: str, walk_root: str) -> bool:
|
|||||||
|
|
||||||
REVIEWER_TASKS = frozenset({
|
REVIEWER_TASKS = frozenset({
|
||||||
"review_pr",
|
"review_pr",
|
||||||
"merge_pr",
|
|
||||||
"blind_pr_queue_review",
|
"blind_pr_queue_review",
|
||||||
"pr_queue_cleanup",
|
"pr_queue_cleanup",
|
||||||
"pr-queue-cleanup",
|
"pr-queue-cleanup",
|
||||||
@@ -63,6 +62,10 @@ REVIEWER_TASKS = frozenset({
|
|||||||
"approve_pr",
|
"approve_pr",
|
||||||
})
|
})
|
||||||
|
|
||||||
|
MERGER_TASKS = frozenset({
|
||||||
|
"merge_pr",
|
||||||
|
})
|
||||||
|
|
||||||
AUTHOR_TASKS = frozenset({
|
AUTHOR_TASKS = frozenset({
|
||||||
"create_issue",
|
"create_issue",
|
||||||
"comment_issue",
|
"comment_issue",
|
||||||
@@ -98,7 +101,7 @@ TASK_REQUIRED_ROLE = {
|
|||||||
"address_pr_change_requests": "author",
|
"address_pr_change_requests": "author",
|
||||||
"delete_branch": "author",
|
"delete_branch": "author",
|
||||||
"review_pr": "reviewer",
|
"review_pr": "reviewer",
|
||||||
"merge_pr": "reviewer",
|
"merge_pr": "merger",
|
||||||
"blind_pr_queue_review": "reviewer",
|
"blind_pr_queue_review": "reviewer",
|
||||||
"pr_queue_cleanup": "reviewer",
|
"pr_queue_cleanup": "reviewer",
|
||||||
"pr-queue-cleanup": "reviewer",
|
"pr-queue-cleanup": "reviewer",
|
||||||
@@ -128,6 +131,10 @@ WRONG_ROLE_RECONCILER_MSG = (
|
|||||||
"MCP namespace/profile with exact close capability."
|
"MCP namespace/profile with exact close capability."
|
||||||
)
|
)
|
||||||
|
|
||||||
|
WRONG_ROLE_MERGER_MSG = (
|
||||||
|
"Wrong role/session for merger task. Launch merger MCP namespace."
|
||||||
|
)
|
||||||
|
|
||||||
_session_last_route: dict | None = None
|
_session_last_route: dict | None = None
|
||||||
|
|
||||||
|
|
||||||
@@ -243,6 +250,25 @@ def route_task_session(
|
|||||||
_record_route(result)
|
_record_route(result)
|
||||||
return result
|
return result
|
||||||
|
|
||||||
|
if required_role == "merger":
|
||||||
|
result = {
|
||||||
|
"task_type": task_type,
|
||||||
|
"required_role": required_role,
|
||||||
|
"active_role": active_role_kind,
|
||||||
|
"active_profile": active_profile,
|
||||||
|
"route_result": ROUTE_WRONG_ROLE,
|
||||||
|
"downstream_allowed": False,
|
||||||
|
"reasons": [
|
||||||
|
WRONG_ROLE_MERGER_MSG,
|
||||||
|
"Merger tasks cannot run in author or reviewer sessions.",
|
||||||
|
],
|
||||||
|
"message": WRONG_ROLE_MERGER_MSG,
|
||||||
|
"runtime_switching_supported": runtime_switching_supported,
|
||||||
|
"profile_switch_blocked": not runtime_switching_supported,
|
||||||
|
}
|
||||||
|
_record_route(result)
|
||||||
|
return result
|
||||||
|
|
||||||
if required_role == "author":
|
if required_role == "author":
|
||||||
route = ROUTE_TO_AUTHOR
|
route = ROUTE_TO_AUTHOR
|
||||||
message = (
|
message = (
|
||||||
|
|||||||
@@ -287,6 +287,49 @@ class TestResolveTaskCapability(unittest.TestCase):
|
|||||||
self.assertEqual(res["required_operation_permission"], "gitea.pr.merge")
|
self.assertEqual(res["required_operation_permission"], "gitea.pr.merge")
|
||||||
self.assertEqual(res["required_role_kind"], "merger")
|
self.assertEqual(res["required_role_kind"], "merger")
|
||||||
|
|
||||||
|
@patch("mcp_server.api_request", return_value={"login": "reviewer-user"})
|
||||||
|
@patch("mcp_server.get_auth_header", return_value="token reviewer-pass")
|
||||||
|
def test_reviewer_role_with_merge_permission_still_cannot_merge(self, _auth, _api):
|
||||||
|
# #539: exact permission alone is insufficient. The active profile
|
||||||
|
# role must also be merger for merge_pr.
|
||||||
|
config = json.loads(json.dumps(CONFIG_RESOLVER))
|
||||||
|
config["profiles"]["reviewer-with-merge"] = {
|
||||||
|
"enabled": True,
|
||||||
|
"context": "ctx",
|
||||||
|
"role": "reviewer",
|
||||||
|
"username": "reviewer-user",
|
||||||
|
"auth": {"type": "env", "name": "GITEA_TOKEN_REVIEWER"},
|
||||||
|
"allowed_operations": [
|
||||||
|
"gitea.read", "gitea.pr.review", "gitea.pr.approve",
|
||||||
|
"gitea.pr.merge",
|
||||||
|
],
|
||||||
|
"forbidden_operations": ["gitea.pr.create", "gitea.branch.push"],
|
||||||
|
"execution_profile": "reviewer-with-merge",
|
||||||
|
}
|
||||||
|
self._write_config(config)
|
||||||
|
with patch.dict(os.environ, self._env("reviewer-with-merge")):
|
||||||
|
res = mcp_server.gitea_resolve_task_capability(
|
||||||
|
task="merge_pr", remote="prgs"
|
||||||
|
)
|
||||||
|
self.assertEqual(res["required_role_kind"], "merger")
|
||||||
|
self.assertEqual(res["active_role_kind"], "reviewer")
|
||||||
|
self.assertTrue(res["active_profile_permission_allowed"])
|
||||||
|
self.assertFalse(res["allowed_in_current_session"])
|
||||||
|
self.assertTrue(res["stop_required"])
|
||||||
|
self.assertIn("cannot perform merger task", " ".join(res["task_role_guidance"]))
|
||||||
|
|
||||||
|
@patch("mcp_server.api_request", return_value={"login": "merger-user"})
|
||||||
|
@patch("mcp_server.get_auth_header", return_value="token merger-pass")
|
||||||
|
def test_merger_profile_can_resolve_merge_task(self, _auth, _api):
|
||||||
|
with patch.dict(os.environ, self._env("merger-profile")):
|
||||||
|
res = mcp_server.gitea_resolve_task_capability(
|
||||||
|
task="merge_pr", remote="prgs"
|
||||||
|
)
|
||||||
|
self.assertEqual(res["required_role_kind"], "merger")
|
||||||
|
self.assertEqual(res["active_role_kind"], "merger")
|
||||||
|
self.assertTrue(res["allowed_in_current_session"])
|
||||||
|
self.assertFalse(res["stop_required"])
|
||||||
|
|
||||||
@patch("mcp_server.api_request")
|
@patch("mcp_server.api_request")
|
||||||
def test_self_review_blocked_structured(self, mock_api):
|
def test_self_review_blocked_structured(self, mock_api):
|
||||||
# Test that self-approve (self-review gate) is blocked and returns structured reasons
|
# Test that self-approve (self-review gate) is blocked and returns structured reasons
|
||||||
|
|||||||
@@ -52,6 +52,20 @@ CONFIG = {
|
|||||||
],
|
],
|
||||||
"execution_profile": "prgs-reviewer",
|
"execution_profile": "prgs-reviewer",
|
||||||
},
|
},
|
||||||
|
"prgs-merger": {
|
||||||
|
"enabled": True,
|
||||||
|
"context": "ctx",
|
||||||
|
"role": "merger",
|
||||||
|
"username": "sysadmin",
|
||||||
|
"auth": {"type": "env", "name": "GITEA_TOKEN_MERGER"},
|
||||||
|
"allowed_operations": [
|
||||||
|
"gitea.read", "gitea.pr.merge", "gitea.issue.comment",
|
||||||
|
],
|
||||||
|
"forbidden_operations": [
|
||||||
|
"gitea.pr.create", "gitea.branch.push", "gitea.pr.approve",
|
||||||
|
],
|
||||||
|
"execution_profile": "prgs-merger",
|
||||||
|
},
|
||||||
"prgs-reconciler": {
|
"prgs-reconciler": {
|
||||||
"enabled": True,
|
"enabled": True,
|
||||||
"context": "ctx",
|
"context": "ctx",
|
||||||
@@ -104,6 +118,7 @@ class TestRoleSessionRouter(unittest.TestCase):
|
|||||||
"GITEA_MCP_PROFILE": profile,
|
"GITEA_MCP_PROFILE": profile,
|
||||||
"GITEA_TOKEN_AUTHOR": "author-pass",
|
"GITEA_TOKEN_AUTHOR": "author-pass",
|
||||||
"GITEA_TOKEN_REVIEWER": "reviewer-pass",
|
"GITEA_TOKEN_REVIEWER": "reviewer-pass",
|
||||||
|
"GITEA_TOKEN_MERGER": "merger-pass",
|
||||||
"GITEA_TOKEN_RECONCILER": "reconciler-pass",
|
"GITEA_TOKEN_RECONCILER": "reconciler-pass",
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -227,6 +242,31 @@ class TestRoleSessionRouter(unittest.TestCase):
|
|||||||
self.assertEqual(route["route_result"], role_session_router.ROUTE_ALLOWED)
|
self.assertEqual(route["route_result"], role_session_router.ROUTE_ALLOWED)
|
||||||
self.assertTrue(route["downstream_allowed"])
|
self.assertTrue(route["downstream_allowed"])
|
||||||
|
|
||||||
|
@patch("mcp_server.api_request", return_value={"login": "sysadmin"})
|
||||||
|
@patch("mcp_server.get_auth_header", return_value="token reviewer-pass")
|
||||||
|
def test_merge_task_under_reviewer_profile_wrong_role_stop(self, _auth, _api):
|
||||||
|
# #539: a reviewer session must not pass the pre-task merge route
|
||||||
|
# even if its config accidentally includes gitea.pr.merge.
|
||||||
|
with patch.dict(os.environ, self._env("prgs-reviewer")):
|
||||||
|
route = mcp_server.gitea_route_task_session(
|
||||||
|
task_type="merge_pr", remote="prgs"
|
||||||
|
)
|
||||||
|
self.assertEqual(route["required_role"], "merger")
|
||||||
|
self.assertEqual(route["route_result"], role_session_router.ROUTE_WRONG_ROLE)
|
||||||
|
self.assertFalse(route["downstream_allowed"])
|
||||||
|
self.assertIn("merger", route["message"].lower())
|
||||||
|
|
||||||
|
@patch("mcp_server.api_request", return_value={"login": "sysadmin"})
|
||||||
|
@patch("mcp_server.get_auth_header", return_value="token merger-pass")
|
||||||
|
def test_merge_task_under_merger_profile_allowed(self, _auth, _api):
|
||||||
|
with patch.dict(os.environ, self._env("prgs-merger")):
|
||||||
|
route = mcp_server.gitea_route_task_session(
|
||||||
|
task_type="merge_pr", remote="prgs"
|
||||||
|
)
|
||||||
|
self.assertEqual(route["required_role"], "merger")
|
||||||
|
self.assertEqual(route["route_result"], role_session_router.ROUTE_ALLOWED)
|
||||||
|
self.assertTrue(route["downstream_allowed"])
|
||||||
|
|
||||||
@patch("mcp_server.api_request", return_value={"login": "sysadmin"})
|
@patch("mcp_server.api_request", return_value={"login": "sysadmin"})
|
||||||
@patch("mcp_server.get_auth_header", return_value="token reviewer-pass")
|
@patch("mcp_server.get_auth_header", return_value="token reviewer-pass")
|
||||||
def test_author_task_under_reviewer_profile_routes_to_author(self, _auth, _api):
|
def test_author_task_under_reviewer_profile_routes_to_author(self, _auth, _api):
|
||||||
@@ -334,4 +374,4 @@ class TestCheckMidMerge(unittest.TestCase):
|
|||||||
|
|
||||||
|
|
||||||
if __name__ == "__main__":
|
if __name__ == "__main__":
|
||||||
unittest.main()
|
unittest.main()
|
||||||
|
|||||||
Reference in New Issue
Block a user