Skip to content

Commit 5080ecd

Browse files
committed
Honor group-derived roles in unprivileged tool access check
DynamicToolManager.ensure_can_use_unprivileged_tool only joined UserRoleAssociation directly, so users granted USER_TOOL_EXECUTE via group membership were denied access. Delegate to User.all_roles(), the project-wide single source of truth for "every role this user has, direct or via groups", instead of maintaining a parallel SQL query that already drifted. Also adds an api regression test exercising the group-inherited path.
1 parent 4cafd91 commit 5080ecd

3 files changed

Lines changed: 29 additions & 11 deletions

File tree

lib/galaxy/managers/tools.py

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,6 @@
99
from uuid import UUID
1010

1111
from sqlalchemy import (
12-
exists,
13-
false,
1412
select,
1513
sql,
1614
true,
@@ -82,15 +80,7 @@ class DynamicToolManager(ModelManager[DynamicTool]):
8280
model_class = DynamicTool
8381

8482
def ensure_can_use_unprivileged_tool(self, user: model.User):
85-
stmt = select(
86-
exists().where(
87-
model.UserRoleAssociation.user_id == user.id,
88-
model.UserRoleAssociation.role_id == model.Role.id,
89-
model.Role.type == model.Role.types.USER_TOOL_EXECUTE,
90-
model.Role.deleted == false(),
91-
)
92-
)
93-
if not self.session().execute(stmt).scalar():
83+
if not any(role.type == model.Role.types.USER_TOOL_EXECUTE and not role.deleted for role in user.all_roles()):
9484
raise exceptions.InsufficientPermissionsException("User is not allowed to run unprivileged tools")
9585

9686
def get_tool_by_id_or_uuid(self, id_or_uuid: Union[int, str]) -> Union[DynamicTool, None]:

lib/galaxy_test/api/test_unprivileged_tools.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,14 @@ def test_create_unprivileged(self):
2929
assert dynamic_tool["uuid"], "Dynamic tool UUID not found in response"
3030
assert dynamic_tool["representation"]["name"] == TOOL_WITH_SHELL_COMMAND["name"]
3131

32+
def test_create_unprivileged_with_group_inherited_role(self):
33+
# Regression: USER_TOOL_EXECUTE granted via group membership must also allow access,
34+
# not only direct UserRoleAssociation.
35+
with self.dataset_populator.user_tool_execute_permissions_via_group():
36+
dynamic_tool = self.dataset_populator.create_unprivileged_tool(UserToolSource(**TOOL_WITH_SHELL_COMMAND))
37+
assert dynamic_tool["uuid"], "Dynamic tool UUID not found in response"
38+
assert dynamic_tool["representation"]["name"] == TOOL_WITH_SHELL_COMMAND["name"]
39+
3240
def test_list_unprivileged(self):
3341
with self.dataset_populator.user_tool_execute_permissions():
3442
dynamic_tool = self.dataset_populator.create_unprivileged_tool(UserToolSource(**TOOL_WITH_SHELL_COMMAND))

lib/galaxy_test/base/populators.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1600,6 +1600,26 @@ def user_tool_execute_permissions(self):
16001600
self._delete(f"roles/{role['id']}", admin=True).raise_for_status()
16011601
self._post(f"roles/{role['id']}/purge", admin=True).raise_for_status()
16021602

1603+
@contextlib.contextmanager
1604+
def user_tool_execute_permissions_via_group(self):
1605+
# Grant USER_TOOL_EXECUTE indirectly: role has no direct user, group binds user to role.
1606+
role = self.create_role([], role_type="user_tool_execute")
1607+
group_payload = {
1608+
"name": self.get_random_name(prefix="testpop-utx-group"),
1609+
"user_ids": [self.user_id()],
1610+
"role_ids": [role["id"]],
1611+
}
1612+
group_response = self._post("groups", data=group_payload, admin=True, json=True)
1613+
assert group_response.status_code == 200
1614+
group = group_response.json()[0]
1615+
try:
1616+
yield
1617+
finally:
1618+
self._delete(f"groups/{group['id']}", admin=True).raise_for_status()
1619+
self._post(f"groups/{group['id']}/purge", admin=True).raise_for_status()
1620+
self._delete(f"roles/{role['id']}", admin=True).raise_for_status()
1621+
self._post(f"roles/{role['id']}/purge", admin=True).raise_for_status()
1622+
16031623
def create_quota(self, quota_payload: dict) -> dict:
16041624
using_requirement("admin")
16051625
quota_response = self._post("quotas", data=quota_payload, admin=True, json=True)

0 commit comments

Comments
 (0)