From 6ea49343eb2e5e0ff148c11a26ffd467774bce99 Mon Sep 17 00:00:00 2001 From: John Davis Date: Tue, 8 Oct 2024 14:08:00 -0400 Subject: [PATCH 01/26] Remove unique constraint from role name, add not null constraint --- lib/galaxy/model/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index bcf956d2aae4..852bd544cd60 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -3750,7 +3750,7 @@ class Role(Base, Dictifiable, RepresentById): id: Mapped[int] = mapped_column(primary_key=True) create_time: Mapped[datetime] = mapped_column(default=now, nullable=True) update_time: Mapped[datetime] = mapped_column(default=now, onupdate=now, nullable=True) - name: Mapped[Optional[str]] = mapped_column(String(255), index=True, unique=True) + name: Mapped[str] = mapped_column(String(255), index=True) description: Mapped[Optional[str]] = mapped_column(TEXT) type: Mapped[Optional[str]] = mapped_column(String(40), index=True) deleted: Mapped[Optional[bool]] = mapped_column(index=True, default=False) From 4186c48710c137885b45d8b83b5432ba7c7ecf65 Mon Sep 17 00:00:00 2001 From: John Davis Date: Tue, 8 Oct 2024 14:20:23 -0400 Subject: [PATCH 02/26] Add migration: remove unique constraint from role name, add not null constraint --- ...remove_unique_constraint_from_role_name.py | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 lib/galaxy/model/migrations/alembic/versions_gxy/9a5207190a4d_remove_unique_constraint_from_role_name.py diff --git a/lib/galaxy/model/migrations/alembic/versions_gxy/9a5207190a4d_remove_unique_constraint_from_role_name.py b/lib/galaxy/model/migrations/alembic/versions_gxy/9a5207190a4d_remove_unique_constraint_from_role_name.py new file mode 100644 index 000000000000..4c4f31fb3ba0 --- /dev/null +++ b/lib/galaxy/model/migrations/alembic/versions_gxy/9a5207190a4d_remove_unique_constraint_from_role_name.py @@ -0,0 +1,40 @@ +"""Remove unique constraint from role name, add not null constraint + +Revision ID: 9a5207190a4d +Revises: a99a5b52ccb8 +Create Date: 2024-10-08 14:08:28.418055 + +""" + +from galaxy.model.database_object_names import build_index_name +from galaxy.model.migrations.util import ( + alter_column, + create_index, + drop_index, + transaction, +) + +# revision identifiers, used by Alembic. +revision = "9a5207190a4d" +down_revision = "a99a5b52ccb8" +branch_labels = None +depends_on = None + + +table_name = "role" +column_name = "name" +index_name = build_index_name(table_name, [column_name]) + + +def upgrade(): + with transaction(): + drop_index(index_name, table_name) + alter_column(table_name, column_name, nullable=False) + create_index(index_name, table_name, [column_name]) + + +def downgrade(): + with transaction(): + drop_index(index_name, table_name) + alter_column(table_name, column_name, nullable=True) + create_index(index_name, table_name, [column_name], unique=True) From 62a8b90f8aab45441b5f6922351b8a5a20cc0848 Mon Sep 17 00:00:00 2001 From: John Davis Date: Mon, 28 Oct 2024 17:05:20 -0400 Subject: [PATCH 03/26] Overwrite role name w/hybrid property --- lib/galaxy/managers/roles.py | 6 +++++- lib/galaxy/model/__init__.py | 16 +++++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/managers/roles.py b/lib/galaxy/managers/roles.py index 8864f8900e16..02448c1a4fbc 100644 --- a/lib/galaxy/managers/roles.py +++ b/lib/galaxy/managers/roles.py @@ -84,7 +84,11 @@ def create_role(self, trans: ProvidesUserContext, role_definition_model: RoleDef user_ids = role_definition_model.user_ids or [] group_ids = role_definition_model.group_ids or [] - stmt = select(Role).where(Role.name == name).limit(1) + stmt = ( + select(Role) + .where(Role.name == name) # type:ignore[arg-type,comparison-overlap] # Role.name is a SA hybrid property + .limit(1) + ) if trans.sa_session.scalars(stmt).first(): raise Conflict(f"A role with that name already exists [{name}]") diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index 852bd544cd60..7f81fb1dd02a 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -106,6 +106,7 @@ association_proxy, AssociationProxy, ) +from sqlalchemy.ext.hybrid import hybrid_property from sqlalchemy.ext.orderinglist import ordering_list from sqlalchemy.orm import ( aliased, @@ -3750,7 +3751,7 @@ class Role(Base, Dictifiable, RepresentById): id: Mapped[int] = mapped_column(primary_key=True) create_time: Mapped[datetime] = mapped_column(default=now, nullable=True) update_time: Mapped[datetime] = mapped_column(default=now, onupdate=now, nullable=True) - name: Mapped[str] = mapped_column(String(255), index=True) + _name: Mapped[str] = mapped_column("name", String(255), index=True) description: Mapped[Optional[str]] = mapped_column(TEXT) type: Mapped[Optional[str]] = mapped_column(String(40), index=True) deleted: Mapped[Optional[bool]] = mapped_column(index=True, default=False) @@ -3769,6 +3770,19 @@ class types(str, Enum): ADMIN = "admin" SHARING = "sharing" + @hybrid_property + def name(self): + if self.type == Role.types.PRIVATE: + user_assocs = self.users + assert len(user_assocs) == 1, f"Did not find exactly one user for private role {self}" + return user_assocs[0].user.email + else: + return self._name + + @name.setter # type:ignore[no-redef] # property setter + def name(self, name): + self._name = name + def __init__(self, name=None, description=None, type=types.SYSTEM, deleted=False): self.name = name self.description = description From 2fce86e32de03a2a74efb2fcd1912a06c89da4e8 Mon Sep 17 00:00:00 2001 From: John Davis Date: Tue, 8 Oct 2024 14:47:34 -0400 Subject: [PATCH 04/26] Assign default role name based on role type Now that role name doesn't have to be unique, we don't want to pass args like "private role" or "shared role" on role creation. --- lib/galaxy/model/__init__.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index 7f81fb1dd02a..e7ec635802d2 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -3770,6 +3770,10 @@ class types(str, Enum): ADMIN = "admin" SHARING = "sharing" + @staticmethod + def default_name(role_type): + return f"{role_type.value} role" + @hybrid_property def name(self): if self.type == Role.types.PRIVATE: @@ -3784,7 +3788,7 @@ def name(self, name): self._name = name def __init__(self, name=None, description=None, type=types.SYSTEM, deleted=False): - self.name = name + self.name = name or Role.default_name(type) self.description = description self.type = type self.deleted = deleted From 3e6191fbe3b5b1f18c6de2dfd413e9c2a6e34b19 Mon Sep 17 00:00:00 2001 From: John Davis Date: Tue, 29 Oct 2024 10:12:21 -0400 Subject: [PATCH 05/26] Search by column name, not property --- lib/galaxy/webapps/galaxy/controllers/admin.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/controllers/admin.py b/lib/galaxy/webapps/galaxy/controllers/admin.py index b92e37c12786..13921485ff18 100644 --- a/lib/galaxy/webapps/galaxy/controllers/admin.py +++ b/lib/galaxy/webapps/galaxy/controllers/admin.py @@ -190,6 +190,7 @@ def get_value(self, trans, grid, role): ] def apply_query_filter(self, query, **kwargs): + # Note: we use Role._name (the column), not Role.name (which is a property) INDEX_SEARCH_FILTERS = { "description": "description", "name": "name", @@ -204,8 +205,10 @@ def apply_query_filter(self, query, **kwargs): key = term.filter q = term.text if key == "name": - query = query.filter(text_column_filter(self.model_class.name, term)) + pass + query = query.filter(text_column_filter(self.model_class._name, term)) if key == "description": + pass query = query.filter(text_column_filter(self.model_class.description, term)) elif key == "is": if q == "deleted": @@ -215,7 +218,7 @@ def apply_query_filter(self, query, **kwargs): raw_text_column_filter( [ self.model_class.description, - self.model_class.name, + self.model_class._name, ], term, ) From f623ee851f6de2d01cc317ed2c3147166f3d9644 Mon Sep 17 00:00:00 2001 From: John Davis Date: Tue, 29 Oct 2024 10:51:07 -0400 Subject: [PATCH 06/26] Adjust unit test: role name == user's current email --- test/unit/data/model/db/test_security.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/test/unit/data/model/db/test_security.py b/test/unit/data/model/db/test_security.py index 9a6db937a86b..170d611c678a 100644 --- a/test/unit/data/model/db/test_security.py +++ b/test/unit/data/model/db/test_security.py @@ -34,12 +34,6 @@ def test_private_user_role_assoc_not_affected_by_setting_user_roles(session, mak assert user.email == private_role.name verify_user_associations(user, [], [private_role]) # the only existing association is with the private role - # Update users's email so it's no longer the same as the private role's name. - user.email = user.email + "updated" - session.add(user) - session.commit() - assert user.email != private_role.name - # Delete user roles GalaxyRBACAgent(session).set_user_group_and_role_associations(user, role_ids=[]) # association with private role is preserved @@ -52,12 +46,6 @@ def test_private_user_role_assoc_not_affected_by_setting_role_users(session, mak assert user.email == private_role.name verify_user_associations(user, [], [private_role]) # the only existing association is with the private role - # Update users's email - user.email = user.email + "updated" - session.add(user) - session.commit() - assert user.email != private_role.name - # Update role users GalaxyRBACAgent(session).set_role_user_and_group_associations(private_role, user_ids=[]) # association of private role with user is preserved From 92916527b60abe43cf8c6f50d6c475155a5ec324 Mon Sep 17 00:00:00 2001 From: John Davis Date: Thu, 3 Oct 2024 14:59:22 -0400 Subject: [PATCH 07/26] Factor out roles service from roles api --- lib/galaxy/webapps/galaxy/api/roles.py | 38 ++++--------- lib/galaxy/webapps/galaxy/services/roles.py | 59 +++++++++++++++++++++ 2 files changed, 68 insertions(+), 29 deletions(-) create mode 100644 lib/galaxy/webapps/galaxy/services/roles.py diff --git a/lib/galaxy/webapps/galaxy/api/roles.py b/lib/galaxy/webapps/galaxy/api/roles.py index 166860c92ffd..55f6d9bfece2 100644 --- a/lib/galaxy/webapps/galaxy/api/roles.py +++ b/lib/galaxy/webapps/galaxy/api/roles.py @@ -7,22 +7,18 @@ from fastapi import Body from galaxy.managers.context import ProvidesUserContext -from galaxy.managers.roles import RoleManager -from galaxy.schema.fields import ( - DecodedDatabaseIdField, - Security, -) +from galaxy.schema.fields import DecodedDatabaseIdField from galaxy.schema.schema import ( RoleDefinitionModel, RoleListResponse, RoleModelResponse, ) -from galaxy.webapps.base.controller import url_for from galaxy.webapps.galaxy.api import ( depends, DependsOnTrans, Router, ) +from galaxy.webapps.galaxy.services.roles import RolesService log = logging.getLogger(__name__) @@ -32,48 +28,32 @@ router = Router(tags=["roles"]) -def role_to_model(role): - item = role.to_dict(view="element") - role_id = Security.security.encode_id(role.id) - item["url"] = url_for("role", id=role_id) - return RoleModelResponse(**item) - - @router.cbv class FastAPIRoles: - role_manager: RoleManager = depends(RoleManager) + service: RolesService = depends(RolesService) @router.get("/api/roles") def index(self, trans: ProvidesUserContext = DependsOnTrans) -> RoleListResponse: - roles = self.role_manager.list_displayable_roles(trans) - return RoleListResponse(root=[role_to_model(r) for r in roles]) + return self.service.get_index(trans=trans) @router.get("/api/roles/{id}") def show(self, id: DecodedDatabaseIdField, trans: ProvidesUserContext = DependsOnTrans) -> RoleModelResponse: - role = self.role_manager.get(trans, id) - return role_to_model(role) + return self.service.show(trans, id) @router.post("/api/roles", require_admin=True) def create( self, trans: ProvidesUserContext = DependsOnTrans, role_definition_model: RoleDefinitionModel = Body(...) ) -> RoleModelResponse: - role = self.role_manager.create_role(trans, role_definition_model) - return role_to_model(role) + return self.service.create(trans, role_definition_model) @router.delete("/api/roles/{id}", require_admin=True) def delete(self, id: DecodedDatabaseIdField, trans: ProvidesUserContext = DependsOnTrans) -> RoleModelResponse: - role = self.role_manager.get(trans, id) - role = self.role_manager.delete(trans, role) - return role_to_model(role) + return self.service.delete(trans, id) @router.post("/api/roles/{id}/purge", require_admin=True) def purge(self, id: DecodedDatabaseIdField, trans: ProvidesUserContext = DependsOnTrans) -> RoleModelResponse: - role = self.role_manager.get(trans, id) - role = self.role_manager.purge(trans, role) - return role_to_model(role) + return self.service.purge(trans, id) @router.post("/api/roles/{id}/undelete", require_admin=True) def undelete(self, id: DecodedDatabaseIdField, trans: ProvidesUserContext = DependsOnTrans) -> RoleModelResponse: - role = self.role_manager.get(trans, id) - role = self.role_manager.undelete(trans, role) - return role_to_model(role) + return self.service.undelete(trans, id) diff --git a/lib/galaxy/webapps/galaxy/services/roles.py b/lib/galaxy/webapps/galaxy/services/roles.py new file mode 100644 index 000000000000..afa34fd05a82 --- /dev/null +++ b/lib/galaxy/webapps/galaxy/services/roles.py @@ -0,0 +1,59 @@ +from galaxy.managers.context import ProvidesUserContext +from galaxy.managers.roles import RoleManager +from galaxy.schema.fields import ( + DecodedDatabaseIdField, + Security, +) +from galaxy.schema.schema import ( + RoleDefinitionModel, + RoleListResponse, + RoleModelResponse, +) +from galaxy.security.idencoding import IdEncodingHelper +from galaxy.webapps.base.controller import url_for +from galaxy.webapps.galaxy.services.base import ServiceBase + + +def role_to_model(role): + item = role.to_dict(view="element") + role_id = Security.security.encode_id(role.id) + item["url"] = url_for("role", id=role_id) + return RoleModelResponse(**item) + + +class RolesService(ServiceBase): + + def __init__( + self, + security: IdEncodingHelper, + role_manager: RoleManager, + ): + super().__init__(security) + self.role_manager = role_manager + + def get_index(self, trans: ProvidesUserContext) -> RoleListResponse: + roles = self.role_manager.list_displayable_roles(trans) + return RoleListResponse(root=[role_to_model(r) for r in roles]) + + def show(self, trans: ProvidesUserContext, id: DecodedDatabaseIdField) -> RoleModelResponse: + role = self.role_manager.get(trans, id) + return role_to_model(role) + + def create(self, trans: ProvidesUserContext, role_definition_model: RoleDefinitionModel): + role = self.role_manager.create_role(trans, role_definition_model) + return role_to_model(role) + + def delete(self, trans: ProvidesUserContext, id: DecodedDatabaseIdField) -> RoleModelResponse: + role = self.role_manager.get(trans, id) + role = self.role_manager.delete(trans, role) + return role_to_model(role) + + def purge(self, trans: ProvidesUserContext, id: DecodedDatabaseIdField) -> RoleModelResponse: + role = self.role_manager.get(trans, id) + role = self.role_manager.purge(trans, role) + return role_to_model(role) + + def undelete(self, trans: ProvidesUserContext, id: DecodedDatabaseIdField) -> RoleModelResponse: + role = self.role_manager.get(trans, id) + role = self.role_manager.undelete(trans, role) + return role_to_model(role) From 6dace477a3ebf1c907d06ab6cf13f000a532e035 Mon Sep 17 00:00:00 2001 From: John Davis Date: Thu, 3 Oct 2024 10:04:20 -0400 Subject: [PATCH 08/26] Add users api endpoint for user-roles --- lib/galaxy/webapps/galaxy/api/users.py | 14 ++++++++++++++ lib/galaxy/webapps/galaxy/services/users.py | 7 +++++++ 2 files changed, 21 insertions(+) diff --git a/lib/galaxy/webapps/galaxy/api/users.py b/lib/galaxy/webapps/galaxy/api/users.py index 9579fa63f79a..c9d4f8bd50ee 100644 --- a/lib/galaxy/webapps/galaxy/api/users.py +++ b/lib/galaxy/webapps/galaxy/api/users.py @@ -57,6 +57,7 @@ FlexibleUserIdType, MaybeLimitedUserModel, RemoteUserCreationPayload, + RoleListResponse, UserBeaconSetting, UserCreationPayload, UserDeletionPayload, @@ -730,6 +731,19 @@ def send_activation_email( if not self.service.user_manager.send_activation_email(trans, user.email, user.username): raise exceptions.MessageException("Unable to send activation email.") + @router.get( + "/api/users/{user_id}/roles", + name="get user roles", + description="Return a collection of roles associated with this user. Only admins can see user roles.", + require_admin=True, + ) + def get_user_roles( + self, + user_id: UserIdPathParam, + trans: ProvidesUserContext = DependsOnTrans, + ) -> RoleListResponse: + return self.service.get_user_roles(trans=trans, user_id=user_id) + class UserAPIController(BaseGalaxyAPIController, UsesTagsMixin, BaseUIController, UsesFormDefinitionsMixin): service: UsersService = depends(UsersService) diff --git a/lib/galaxy/webapps/galaxy/services/users.py b/lib/galaxy/webapps/galaxy/services/users.py index 84c1f4a12978..1d5705b34bac 100644 --- a/lib/galaxy/webapps/galaxy/services/users.py +++ b/lib/galaxy/webapps/galaxy/services/users.py @@ -30,6 +30,7 @@ FlexibleUserIdType, LimitedUserModel, MaybeLimitedUserModel, + RoleListResponse, UserModel, ) from galaxy.security.idencoding import IdEncodingHelper @@ -37,6 +38,7 @@ async_task_summary, ServiceBase, ) +from galaxy.webapps.galaxy.services.roles import role_to_model class UsersService(ServiceBase): @@ -248,3 +250,8 @@ def get_index( else: rval.append(UserModel(**user_dict)) return rval + + def get_user_roles(self, trans, user_id): + user = self.get_user(trans, user_id) + roles = [ura.role for ura in user.roles] + return RoleListResponse(root=[role_to_model(r) for r in roles]) From 98044ad7dce0573fa6646adc29adcbbea8bf0e42 Mon Sep 17 00:00:00 2001 From: John Davis Date: Thu, 3 Oct 2024 15:04:09 -0400 Subject: [PATCH 09/26] Change how a populator gets a user's private role Because we no longer can match role name to user email --- lib/galaxy_test/base/populators.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/galaxy_test/base/populators.py b/lib/galaxy_test/base/populators.py index 5b8e46998c22..35bef2c88c29 100644 --- a/lib/galaxy_test/base/populators.py +++ b/lib/galaxy_test/base/populators.py @@ -145,6 +145,8 @@ SKIP_FLAKEY_TESTS_ON_ERROR = os.environ.get("GALAXY_TEST_SKIP_FLAKEY_TESTS_ON_ERROR", None) +PRIVATE_ROLE_TYPE = "private" + def flakey(method): @wraps(method) @@ -1277,11 +1279,13 @@ def user_id(self) -> str: return users[0]["id"] def user_private_role_id(self) -> str: - user_email = self.user_email() - roles = self.get_roles() - users_roles = [r for r in roles if r["name"] == user_email] - assert len(users_roles) == 1, f"Did not find exactly one role for email {user_email} - {users_roles}" - role = users_roles[0] + userid = self.user_id() + response = self._get(f"users/{userid}/roles", admin=True) + assert response.status_code == 200 + roles = response.json() + private_roles = [r for r in roles if r["type"] == PRIVATE_ROLE_TYPE] + assert len(private_roles) == 1, f"Did not find exactly one private role for user {userid} - {private_roles}" + role = private_roles[0] assert "id" in role, role return role["id"] From af5633862cbc30a611ee3ccb9c04bd7695596002 Mon Sep 17 00:00:00 2001 From: John Davis Date: Thu, 3 Oct 2024 15:04:53 -0400 Subject: [PATCH 10/26] Add API test for new user-roles endpoint --- lib/galaxy_test/api/test_users.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/galaxy_test/api/test_users.py b/lib/galaxy_test/api/test_users.py index 8e081fd2389b..21c5c7fca473 100644 --- a/lib/galaxy_test/api/test_users.py +++ b/lib/galaxy_test/api/test_users.py @@ -7,6 +7,7 @@ ) from galaxy_test.base.populators import ( DatasetPopulator, + PRIVATE_ROLE_TYPE, skip_without_tool, ) @@ -20,6 +21,7 @@ class TestUsersApi(ApiTestCase): + @requires_admin @requires_new_user def test_index(self): @@ -356,3 +358,12 @@ def test_manage_beacon_settings(self): response = self._get(f"users/{user_id}/beacon") user_beacon_settings = response.json() assert user_beacon_settings["enabled"] + + @requires_admin + @requires_new_user + def test_user_roles(self): + user = self._setup_user(TEST_USER_EMAIL) + response = self._get(f"users/{user['id']}/roles", admin=True) + user_roles = response.json() + assert len(user_roles) == 1 + assert user_roles[0]["type"] == PRIVATE_ROLE_TYPE From e255a384cde53eeb4b20128aa441a585e72f4934 Mon Sep 17 00:00:00 2001 From: John Davis Date: Tue, 8 Oct 2024 16:52:41 -0400 Subject: [PATCH 11/26] Modify make_role fixture to accommodate downgrading in migration tests --- test/unit/data/model/conftest.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/unit/data/model/conftest.py b/test/unit/data/model/conftest.py index f49454266001..a3daec99bc2e 100644 --- a/test/unit/data/model/conftest.py +++ b/test/unit/data/model/conftest.py @@ -375,6 +375,14 @@ def f(**kwd): @pytest.fixture def make_role(session): def f(**kwd): + # We must specify `name` because after removing the unique constraint + # from role.name (migration 9a5207190a4d) and setting up a default name + # generation for roles that do not receive a name argument that does + # not generate unique names, any migration unit tests that use + # this fixture AFTER DOWNGRADING (like # test_migrations.py::test_349dd9d9aac9) + # would break due to violating that constraint (restored via + # downgrading) without setting name. + kwd["name"] = kwd.get("name") or random_str() model = m.Role(**kwd) write_to_db(session, model) return model From 0e51a52272fd3bf970794f63ab54e208abbc992f Mon Sep 17 00:00:00 2001 From: John Davis Date: Fri, 25 Oct 2024 11:12:07 -0400 Subject: [PATCH 12/26] Move test fixture to conftest --- test/unit/data/model/conftest.py | 18 ++++++++++++++++++ test/unit/data/model/db/test_security.py | 18 ------------------ 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/test/unit/data/model/conftest.py b/test/unit/data/model/conftest.py index a3daec99bc2e..a10f10cb00ce 100644 --- a/test/unit/data/model/conftest.py +++ b/test/unit/data/model/conftest.py @@ -428,6 +428,24 @@ def f(**kwd): return f +@pytest.fixture +def make_user_and_role(session, make_user, make_role, make_user_role_association): + """ + Each user created in Galaxy is assumed to have a private role, such that role.type == Role.types.PRIVATE. + Since we are testing user/group/role associations here, to ensure the correct state of the test database, + we need to ensure that a user is never created without a corresponding private role. + Therefore, we use this fixture instead of make_user (which only creates a user). + """ + + def f(**kwd): + user = make_user(**kwd) + private_role = make_role(type=m.Role.types.PRIVATE) + make_user_role_association(user, private_role) + return user, private_role + + return f + + @pytest.fixture def make_user_item_rating_association(session): def f(assoc_class, user, item, rating): diff --git a/test/unit/data/model/db/test_security.py b/test/unit/data/model/db/test_security.py index 170d611c678a..21ea4d89eb5e 100644 --- a/test/unit/data/model/db/test_security.py +++ b/test/unit/data/model/db/test_security.py @@ -10,24 +10,6 @@ from . import have_same_elements -@pytest.fixture -def make_user_and_role(session, make_user, make_role, make_user_role_association): - """ - Each user created in Galaxy is assumed to have a private role, such that role.name == user.email. - Since we are testing user/group/role associations here, to ensure the correct state of the test database, - we need to ensure that a user is never created without a corresponding private role. - Therefore, we use this fixture instead of make_user (which only creates a user). - """ - - def f(**kwd): - user = make_user() - private_role = make_role(name=user.email, type=Role.types.PRIVATE) - make_user_role_association(user, private_role) - return user, private_role - - return f - - def test_private_user_role_assoc_not_affected_by_setting_user_roles(session, make_user_and_role): # Create user with a private role user, private_role = make_user_and_role() From f8c751a788157a840e977676758275ae3b48a563 Mon Sep 17 00:00:00 2001 From: John Davis Date: Tue, 8 Oct 2024 15:58:43 -0400 Subject: [PATCH 13/26] Do not use user's email in private role name, description Reason: decouple user email from private role naming: emails can be changed or redacted; user id in user-role-association + role type is sufficient to tie a user to a private role. The description (i.e., "this is a private role for a user" is inferrable from the role name ("private role"), which is assigned by default. --- lib/galaxy/model/__init__.py | 5 +---- test/unit/data/test_galaxy_mapping.py | 1 - 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index e7ec635802d2..9871312195d7 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -1230,10 +1230,7 @@ def is_authenticated(self): def attempt_create_private_role(self): session = object_session(self) - role_name = self.email - role_desc = f"Private Role for {self.email}" - role_type = Role.types.PRIVATE - role = Role(name=role_name, description=role_desc, type=role_type) + role = Role(type=Role.types.PRIVATE) assoc = UserRoleAssociation(self, role) session.add(assoc) with transaction(session): diff --git a/test/unit/data/test_galaxy_mapping.py b/test/unit/data/test_galaxy_mapping.py index 60c9c3116942..2e71b5d97b8c 100644 --- a/test/unit/data/test_galaxy_mapping.py +++ b/test/unit/data/test_galaxy_mapping.py @@ -462,7 +462,6 @@ def check_private_role(private_role, email): assert private_role.type == model.Role.types.PRIVATE assert len(private_role.users) == 1 assert private_role.name == email - assert private_role.description == "Private Role for " + email email = "rule_user_1@example.com" u = model.User(email=email, password="password") From a58c417eb42f7bdd24c8e4215f50b55c05bccd07 Mon Sep 17 00:00:00 2001 From: John Davis Date: Tue, 29 Oct 2024 23:04:56 -0400 Subject: [PATCH 14/26] Fix query to look up role by user email --- lib/galaxy/model/security.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/galaxy/model/security.py b/lib/galaxy/model/security.py index a5bc222e2d00..d1e711499491 100644 --- a/lib/galaxy/model/security.py +++ b/lib/galaxy/model/security.py @@ -159,13 +159,20 @@ def get_valid_roles(self, trans, item, query=None, page=None, page_limit=None, i is_public_item = False # Admins can always choose from all non-deleted roles if trans.user_is_admin or trans.app.config.expose_user_email: - if trans.user_is_admin: - stmt = select(Role).where(Role.deleted == false()) - else: - # User is not an admin but the configuration exposes all private roles to all users. - stmt = select(Role).where(and_(Role.deleted == false(), Role.type == Role.types.PRIVATE)) + + stmt = select(Role) + if search_query: - stmt = stmt.where(Role.name.like(search_query, escape="/")) + stmt = stmt.join(Role.users).join(User) # We need to query against user email + stmt = stmt.where( + or_(Role.name.like(search_query, escape="/"), User.email.like(search_query, escape="/")) + ) + + stmt = stmt.where(Role.deleted == false()) + + if not trans.user_is_admin: + # User is not an admin but the configuration exposes all private roles to all users. + stmt = stmt.where(Role.type == Role.types.PRIVATE) count_stmt = select(func.count()).select_from(stmt) total_count = trans.sa_session.scalar(count_stmt) From bc3ed5141e8149ca1cedb45aa427cb3fb9de5067 Mon Sep 17 00:00:00 2001 From: John Davis Date: Wed, 9 Oct 2024 10:06:04 -0400 Subject: [PATCH 15/26] Fix grammar typo in schema --- lib/galaxy/schema/schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/schema/schema.py b/lib/galaxy/schema/schema.py index 0d9646316e59..073c2f5da9fd 100644 --- a/lib/galaxy/schema/schema.py +++ b/lib/galaxy/schema/schema.py @@ -3118,7 +3118,7 @@ class LibraryAvailablePermissions(Model): roles: List[BasicRoleModel] = Field( ..., title="Roles", - description="A list available roles that can be assigned to a particular permission.", + description="A list containing available roles that can be assigned to a particular permission.", ) page: int = Field( ..., From 061aa88a500804aa1fb6d8a2afa4b1cc4066842f Mon Sep 17 00:00:00 2001 From: John Davis Date: Tue, 29 Oct 2024 23:20:04 -0400 Subject: [PATCH 16/26] Rebuild client schema --- client/src/api/schema/schema.ts | 66 ++++++++++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/client/src/api/schema/schema.ts b/client/src/api/schema/schema.ts index 93d99b75fa0a..e1e708e0d568 100644 --- a/client/src/api/schema/schema.ts +++ b/client/src/api/schema/schema.ts @@ -4716,6 +4716,26 @@ export interface paths { patch?: never; trace?: never; }; + "/api/users/{user_id}/roles": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + /** + * Get User Roles + * @description Return a collection of roles associated with this user. Only admins can see user roles. + */ + get: operations["get_user_roles_api_users__user_id__roles_get"]; + put?: never; + post?: never; + delete?: never; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; "/api/users/{user_id}/send_activation_email": { parameters: { query?: never; @@ -13132,7 +13152,7 @@ export interface components { page_limit: number; /** * Roles - * @description A list available roles that can be assigned to a particular permission. + * @description A list containing available roles that can be assigned to a particular permission. */ roles: components["schemas"]["BasicRoleModel"][]; /** @@ -33772,6 +33792,50 @@ export interface operations { }; }; }; + get_user_roles_api_users__user_id__roles_get: { + parameters: { + query?: never; + header?: { + /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */ + "run-as"?: string | null; + }; + path: { + /** @description The ID of the user. */ + user_id: string; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description Successful Response */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["RoleListResponse"]; + }; + }; + /** @description Request Error */ + "4XX": { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["MessageExceptionModel"]; + }; + }; + /** @description Server Error */ + "5XX": { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["MessageExceptionModel"]; + }; + }; + }; + }; send_activation_email_api_users__user_id__send_activation_email_post: { parameters: { query?: never; From 93b22b027b3036e65433e824bd4765f234fa3350 Mon Sep 17 00:00:00 2001 From: John Davis Date: Tue, 1 Oct 2024 14:43:38 -0400 Subject: [PATCH 17/26] Update remote user private role bug fix from 2009 --- lib/galaxy/managers/users.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py index 6e50241ab3d9..fc8644d1ba4e 100644 --- a/lib/galaxy/managers/users.py +++ b/lib/galaxy/managers/users.py @@ -658,11 +658,8 @@ def get_or_create_remote_user(self, remote_user_email): remote_user_email = remote_user_email.lower() user = get_user_by_email(self.session(), remote_user_email, self.app.model.User) if user: - # GVK: June 29, 2009 - This is to correct the behavior of a previous bug where a private - # role and default user / history permissions were not set for remote users. When a - # remote user authenticates, we'll look for this information, and if missing, create it. - if not self.app.security_agent.get_private_user_role(user): - self.app.security_agent.create_private_user_role(user) + # Ensure a private role and default permissions are set for remote users (remote user creation bug existed prior to 2009) + self.app.security_agent.get_private_user_role(user, auto_create=True) if self.app_type == "galaxy": if not user.default_permissions: self.app.security_agent.user_set_default_permissions(user) From 1860e88954085ab9eed428ec60ddc39181c50e86 Mon Sep 17 00:00:00 2001 From: John Davis Date: Tue, 1 Oct 2024 15:55:26 -0400 Subject: [PATCH 18/26] Remove breakpoint() comment --- lib/galaxy/model/security.py | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/galaxy/model/security.py b/lib/galaxy/model/security.py index d1e711499491..687f5e0fa5aa 100644 --- a/lib/galaxy/model/security.py +++ b/lib/galaxy/model/security.py @@ -1533,7 +1533,6 @@ def _set_user_roles(self, user, role_ids): else: delete_stmt = delete_stmt.where(UserRoleAssociation.role_id != private_role.id) role_ids = self._filter_private_roles(role_ids) - # breakpoint() insert_values = [{"user_id": user.id, "role_id": role_id} for role_id in role_ids] self._set_associations(user, UserRoleAssociation, delete_stmt, insert_values) From 16f1d3f0e3cbb8a51dfec9bbb94d2d728b4426d2 Mon Sep 17 00:00:00 2001 From: John Davis Date: Wed, 30 Oct 2024 11:43:49 -0400 Subject: [PATCH 19/26] Fix error in select, add test We must use a union because when we retrieve roles with a query, we check against: 1) role name 2) email of associated user for private roles We factor out this select into a helper method, which we then test extensively. --- lib/galaxy/model/security.py | 76 +++++++++++++--------- test/unit/data/model/db/test_role.py | 97 ++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 32 deletions(-) diff --git a/lib/galaxy/model/security.py b/lib/galaxy/model/security.py index 687f5e0fa5aa..4f5d0951e6f3 100644 --- a/lib/galaxy/model/security.py +++ b/lib/galaxy/model/security.py @@ -159,38 +159,7 @@ def get_valid_roles(self, trans, item, query=None, page=None, page_limit=None, i is_public_item = False # Admins can always choose from all non-deleted roles if trans.user_is_admin or trans.app.config.expose_user_email: - - stmt = select(Role) - - if search_query: - stmt = stmt.join(Role.users).join(User) # We need to query against user email - stmt = stmt.where( - or_(Role.name.like(search_query, escape="/"), User.email.like(search_query, escape="/")) - ) - - stmt = stmt.where(Role.deleted == false()) - - if not trans.user_is_admin: - # User is not an admin but the configuration exposes all private roles to all users. - stmt = stmt.where(Role.type == Role.types.PRIVATE) - - count_stmt = select(func.count()).select_from(stmt) - total_count = trans.sa_session.scalar(count_stmt) - - if limit is not None: - # Takes the least number of results from beginning that includes the requested page - stmt = stmt.order_by(Role.name).limit(limit) - page_start = (page * page_limit) - page_limit - page_end = page_start + page_limit - if total_count < page_start + 1: - # Return empty list if there are less results than the requested position - roles = [] - else: - roles = trans.sa_session.scalars(stmt).all() - roles = roles[page_start:page_end] - else: - stmt = stmt.order_by(Role.name) - roles = trans.sa_session.scalars(stmt).all() + roles = _get_valid_roles_case1(trans.sa_session, search_query, trans.user_is_admin, limit, page, page_limit) # Non-admin and public item elif is_public_item: # Add the current user's private role @@ -1814,3 +1783,46 @@ def is_foreign_key_violation(error): # If this is a PostgreSQL foreign key error, then error.orig is an instance of psycopg2.errors.ForeignKeyViolation # and should have an attribute `pgcode` = 23503. return int(getattr(error.orig, "pgcode", -1)) == 23503 + + +def _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit): + """Case: trans.user_is_admin or trans.app.config.expose_user_email""" + stmt = select(Role).where(Role.deleted == false()) + + if not is_admin: + # User is not an admin but the configuration exposes all private roles to all users, + # so only private roles are returned. + stmt = stmt.where(Role.type == Role.types.PRIVATE) + + if search_query: + stmt = stmt.where(Role.name.like(search_query, escape="/")) + + # Also check against user emails for associated users of private roles ONLY + stmt2 = ( + select(Role) + .join(Role.users) + .join(User) + .where(and_(Role.type == Role.types.PRIVATE, User.email.like(search_query, escape="/"))) + ) + stmt = stmt.union(stmt2) + + count_stmt = select(func.count()).select_from(stmt) + total_count = session.scalar(count_stmt) + + stmt = stmt.order_by(Role.name) + + if limit is not None: + # Takes the least number of results from beginning that includes the requested page + stmt = stmt.limit(limit) + page_start = (page * page_limit) - page_limit + page_end = page_start + page_limit + if total_count < page_start + 1: + # Return empty list if there are less results than the requested position + return [] + + stmt = select(Role).from_statement(stmt) + roles = session.scalars(stmt).all() + if limit is not None: + roles = roles[page_start:page_end] + + return roles diff --git a/test/unit/data/model/db/test_role.py b/test/unit/data/model/db/test_role.py index 213314c5c609..248308a277df 100644 --- a/test/unit/data/model/db/test_role.py +++ b/test/unit/data/model/db/test_role.py @@ -4,6 +4,7 @@ get_private_user_role, get_roles_by_ids, ) +from galaxy.model.security import _get_valid_roles_case1 from . import have_same_elements @@ -42,3 +43,99 @@ def test_get_roles_by_ids(session, make_role): roles2 = get_roles_by_ids(session, ids) expected = [r1, r2, r3] have_same_elements(roles2, expected) + + +def test_get_falid_roles_case1(session, make_user_and_role, make_user, make_role, make_user_role_association): + # Make 3 users with private roles + ( + u1, + rp1, + ) = make_user_and_role(email="foo1@x.com") + ( + u2, + rp2, + ) = make_user_and_role(email="foo2@x.com") + ( + u3, + rp3, + ) = make_user_and_role(email="bar@x.com") + + # Make 2 sharing roles + rs1 = make_role(type="sharing", name="sharing role for u1") + make_user_role_association(user=u1, role=rs1) + rs2 = make_role(type="sharing", name="sharing role for u2") + make_user_role_association(user=u2, role=rs2) + + # Make 4 admin roles + ra1 = make_role(type="admin", name="admin role1") + make_user_role_association(user=u1, role=ra1) + make_user_role_association(user=u2, role=ra1) + ra2 = make_role(type="admin", name="admin role2") + make_user_role_association(user=u1, role=ra2) + make_user_role_association(user=u2, role=ra2) + + limit, page, page_limit = 1000, 1, 1000 + + is_admin = True + + search_query = None + roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + assert len(roles) == 7 # all roles returned + + search_query = "foo%" + roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + assert len(roles) == 2 + assert rp1 in roles + assert rp2 in roles + + search_query = "foo1%" + roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + assert len(roles) == 1 + assert roles[0] == rp1 + + search_query = "sharing%" + roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + assert len(roles) == 2 + assert rs1 in roles + assert rs2 in roles + + search_query = "sharing role for u1%" + roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + assert len(roles) == 1 + assert roles[0] == rs1 + + search_query = "admin role%" + roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + assert len(roles) == 2 + assert ra1 in roles + assert ra2 in roles + + search_query = "admin role1%" + roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + assert len(roles) == 1 + assert roles[0] == ra1 + + is_admin = False # non admins should see only private roles + + search_query = None + roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + assert len(roles) == 3 + + search_query = "foo%" + roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + assert len(roles) == 2 + assert rp1 in roles + assert rp2 in roles + + search_query = "foo1%" + roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + assert len(roles) == 1 + assert roles[0] == rp1 + + search_query = "sharing%" + roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + assert len(roles) == 0 + + search_query = "admin role%" + roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + assert len(roles) == 0 From 4a7eb0a6ecae75c2b8b19bcfef16032cd73a23f7 Mon Sep 17 00:00:00 2001 From: John Davis Date: Thu, 31 Oct 2024 09:25:48 -0400 Subject: [PATCH 20/26] Fix typo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: David López <46503462+davelopez@users.noreply.github.com> --- test/unit/data/model/db/test_role.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/data/model/db/test_role.py b/test/unit/data/model/db/test_role.py index 248308a277df..fa39c8398235 100644 --- a/test/unit/data/model/db/test_role.py +++ b/test/unit/data/model/db/test_role.py @@ -45,7 +45,7 @@ def test_get_roles_by_ids(session, make_role): have_same_elements(roles2, expected) -def test_get_falid_roles_case1(session, make_user_and_role, make_user, make_role, make_user_role_association): +def test_get_valid_roles_case1(session, make_user_and_role, make_user, make_role, make_user_role_association): # Make 3 users with private roles ( u1, From 6c59904974b6149ad8b740b352cfb70045e14d94 Mon Sep 17 00:00:00 2001 From: John Davis Date: Thu, 31 Oct 2024 09:26:39 -0400 Subject: [PATCH 21/26] Use a better name for method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: David López <46503462+davelopez@users.noreply.github.com> --- lib/galaxy/model/security.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/model/security.py b/lib/galaxy/model/security.py index 4f5d0951e6f3..58efc025350e 100644 --- a/lib/galaxy/model/security.py +++ b/lib/galaxy/model/security.py @@ -1785,7 +1785,7 @@ def is_foreign_key_violation(error): return int(getattr(error.orig, "pgcode", -1)) == 23503 -def _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit): +def _get_valid_roles_exposed(session, search_query, is_admin, limit, page, page_limit): """Case: trans.user_is_admin or trans.app.config.expose_user_email""" stmt = select(Role).where(Role.deleted == false()) From 4be1eaf692b293606caf7d9af9cbce3ad710caf7 Mon Sep 17 00:00:00 2001 From: John Davis Date: Thu, 31 Oct 2024 09:28:49 -0400 Subject: [PATCH 22/26] Update unit test w/new method name --- test/unit/data/model/db/test_role.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/test/unit/data/model/db/test_role.py b/test/unit/data/model/db/test_role.py index fa39c8398235..1fa49ae4a998 100644 --- a/test/unit/data/model/db/test_role.py +++ b/test/unit/data/model/db/test_role.py @@ -4,7 +4,7 @@ get_private_user_role, get_roles_by_ids, ) -from galaxy.model.security import _get_valid_roles_case1 +from galaxy.model.security import _get_valid_roles_exposed from . import have_same_elements @@ -45,7 +45,7 @@ def test_get_roles_by_ids(session, make_role): have_same_elements(roles2, expected) -def test_get_valid_roles_case1(session, make_user_and_role, make_user, make_role, make_user_role_association): +def test_get_valid_roles_exposed(session, make_user_and_role, make_user, make_role, make_user_role_association): # Make 3 users with private roles ( u1, @@ -79,63 +79,63 @@ def test_get_valid_roles_case1(session, make_user_and_role, make_user, make_role is_admin = True search_query = None - roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + roles = _get_valid_roles_exposed(session, search_query, is_admin, limit, page, page_limit) assert len(roles) == 7 # all roles returned search_query = "foo%" - roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + roles = _get_valid_roles_exposed(session, search_query, is_admin, limit, page, page_limit) assert len(roles) == 2 assert rp1 in roles assert rp2 in roles search_query = "foo1%" - roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + roles = _get_valid_roles_exposed(session, search_query, is_admin, limit, page, page_limit) assert len(roles) == 1 assert roles[0] == rp1 search_query = "sharing%" - roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + roles = _get_valid_roles_exposed(session, search_query, is_admin, limit, page, page_limit) assert len(roles) == 2 assert rs1 in roles assert rs2 in roles search_query = "sharing role for u1%" - roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + roles = _get_valid_roles_exposed(session, search_query, is_admin, limit, page, page_limit) assert len(roles) == 1 assert roles[0] == rs1 search_query = "admin role%" - roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + roles = _get_valid_roles_exposed(session, search_query, is_admin, limit, page, page_limit) assert len(roles) == 2 assert ra1 in roles assert ra2 in roles search_query = "admin role1%" - roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + roles = _get_valid_roles_exposed(session, search_query, is_admin, limit, page, page_limit) assert len(roles) == 1 assert roles[0] == ra1 is_admin = False # non admins should see only private roles search_query = None - roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + roles = _get_valid_roles_exposed(session, search_query, is_admin, limit, page, page_limit) assert len(roles) == 3 search_query = "foo%" - roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + roles = _get_valid_roles_exposed(session, search_query, is_admin, limit, page, page_limit) assert len(roles) == 2 assert rp1 in roles assert rp2 in roles search_query = "foo1%" - roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + roles = _get_valid_roles_exposed(session, search_query, is_admin, limit, page, page_limit) assert len(roles) == 1 assert roles[0] == rp1 search_query = "sharing%" - roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + roles = _get_valid_roles_exposed(session, search_query, is_admin, limit, page, page_limit) assert len(roles) == 0 search_query = "admin role%" - roles = _get_valid_roles_case1(session, search_query, is_admin, limit, page, page_limit) + roles = _get_valid_roles_exposed(session, search_query, is_admin, limit, page, page_limit) assert len(roles) == 0 From 076126980dc3b000993219a380d8ad6f297314fc Mon Sep 17 00:00:00 2001 From: John Davis Date: Thu, 31 Oct 2024 17:00:06 -0400 Subject: [PATCH 23/26] Call new method --- lib/galaxy/model/security.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/model/security.py b/lib/galaxy/model/security.py index 58efc025350e..188cd926e97b 100644 --- a/lib/galaxy/model/security.py +++ b/lib/galaxy/model/security.py @@ -159,7 +159,7 @@ def get_valid_roles(self, trans, item, query=None, page=None, page_limit=None, i is_public_item = False # Admins can always choose from all non-deleted roles if trans.user_is_admin or trans.app.config.expose_user_email: - roles = _get_valid_roles_case1(trans.sa_session, search_query, trans.user_is_admin, limit, page, page_limit) + roles = _get_valid_roles_exposed(trans.sa_session, search_query, trans.user_is_admin, limit, page, page_limit) # Non-admin and public item elif is_public_item: # Add the current user's private role From ff20d0de1e338d9d8704bc8175ba1b3a1ebd7386 Mon Sep 17 00:00:00 2001 From: John Davis Date: Thu, 31 Oct 2024 23:10:41 -0400 Subject: [PATCH 24/26] Fix formatting --- lib/galaxy/model/security.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/model/security.py b/lib/galaxy/model/security.py index 188cd926e97b..85fcb7be8532 100644 --- a/lib/galaxy/model/security.py +++ b/lib/galaxy/model/security.py @@ -159,7 +159,9 @@ def get_valid_roles(self, trans, item, query=None, page=None, page_limit=None, i is_public_item = False # Admins can always choose from all non-deleted roles if trans.user_is_admin or trans.app.config.expose_user_email: - roles = _get_valid_roles_exposed(trans.sa_session, search_query, trans.user_is_admin, limit, page, page_limit) + roles = _get_valid_roles_exposed( + trans.sa_session, search_query, trans.user_is_admin, limit, page, page_limit + ) # Non-admin and public item elif is_public_item: # Add the current user's private role From d2583bbf9417e4155b816741db45fafe3fe5a767 Mon Sep 17 00:00:00 2001 From: John Davis Date: Mon, 18 Nov 2024 10:36:12 -0500 Subject: [PATCH 25/26] Remove leftover debugging code Co-authored-by: Marius van den Beek --- lib/galaxy/webapps/galaxy/controllers/admin.py | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/galaxy/webapps/galaxy/controllers/admin.py b/lib/galaxy/webapps/galaxy/controllers/admin.py index 13921485ff18..1f0cbd1eaadf 100644 --- a/lib/galaxy/webapps/galaxy/controllers/admin.py +++ b/lib/galaxy/webapps/galaxy/controllers/admin.py @@ -205,7 +205,6 @@ def apply_query_filter(self, query, **kwargs): key = term.filter q = term.text if key == "name": - pass query = query.filter(text_column_filter(self.model_class._name, term)) if key == "description": pass From 85a94a8cdda57ecdd67bd2d5f6c381b9e834b9cd Mon Sep 17 00:00:00 2001 From: John Davis Date: Mon, 18 Nov 2024 10:36:32 -0500 Subject: [PATCH 26/26] Remove leftover debugging code (more) Co-authored-by: Marius van den Beek --- lib/galaxy/webapps/galaxy/controllers/admin.py | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/galaxy/webapps/galaxy/controllers/admin.py b/lib/galaxy/webapps/galaxy/controllers/admin.py index 1f0cbd1eaadf..634c668b1f01 100644 --- a/lib/galaxy/webapps/galaxy/controllers/admin.py +++ b/lib/galaxy/webapps/galaxy/controllers/admin.py @@ -207,7 +207,6 @@ def apply_query_filter(self, query, **kwargs): if key == "name": query = query.filter(text_column_filter(self.model_class._name, term)) if key == "description": - pass query = query.filter(text_column_filter(self.model_class.description, term)) elif key == "is": if q == "deleted":