Skip to content

Commit

Permalink
Do not retrieve roles based on string matching
Browse files Browse the repository at this point in the history
Reasons: not needed, query is ambiguos after decoupling user emails from role names
  • Loading branch information
jdavcs committed Oct 9, 2024
1 parent 198f6a5 commit 6f9fe64
Show file tree
Hide file tree
Showing 6 changed files with 3 additions and 39 deletions.
9 changes: 1 addition & 8 deletions lib/galaxy/model/security.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ def get_roles_for_action(self, item, action):
roles.append(item_permission.role)
return roles

def get_valid_roles(self, trans, item, query=None, page=None, page_limit=None, is_library_access=False):
def get_valid_roles(self, trans, item, page=None, page_limit=None, is_library_access=False):
"""
This method retrieves the list of possible roles that user can select
in the item permissions form. Admins can select any role so the
Expand All @@ -138,11 +138,6 @@ def get_valid_roles(self, trans, item, query=None, page=None, page_limit=None, i
sharing roles and any public role (not private and not sharing).
"""
roles = []
if query not in [None, ""]:
query = query.strip().replace("_", "/_").replace("%", "/%").replace("/", "//")
search_query = f"{query}%"
else:
search_query = None
# Limit the query only to get the page needed
if page is not None and page_limit is not None:
limit = page * page_limit
Expand All @@ -164,8 +159,6 @@ def get_valid_roles(self, trans, item, query=None, page=None, page_limit=None, i
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))
if search_query:
stmt = stmt.where(Role.name.like(search_query, escape="/"))

count_stmt = select(func.count()).select_from(stmt)
total_count = trans.sa_session.scalar(count_stmt)
Expand Down
4 changes: 0 additions & 4 deletions lib/galaxy/webapps/galaxy/api/folders.py
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,6 @@ def get_permissions(
page_limit: int = Query(
default=10, title="Page Limit", description="The maximum number of permissions per page when paginating."
),
q: Optional[str] = Query(
None, title="Query", description="Optional search text to retrieve only the roles matching this query."
),
) -> Union[LibraryFolderCurrentPermissions, LibraryAvailablePermissions]:
"""Gets the current or available permissions of a particular library.
The results can be paginated and additionally filtered by a query."""
Expand All @@ -135,7 +132,6 @@ def get_permissions(
scope,
page,
page_limit,
q,
)

@router.post(
Expand Down
4 changes: 0 additions & 4 deletions lib/galaxy/webapps/galaxy/api/libraries.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,9 +172,6 @@ def get_permissions(
page_limit: int = Query(
default=10, title="Page Limit", description="The maximum number of permissions per page when paginating."
),
q: Optional[str] = Query(
None, title="Query", description="Optional search text to retrieve only the roles matching this query."
),
) -> Union[LibraryCurrentPermissions, LibraryAvailablePermissions]:
"""Gets the current or available permissions of a particular library.
The results can be paginated and additionally filtered by a query."""
Expand All @@ -185,7 +182,6 @@ def get_permissions(
is_library_access,
page,
page_limit,
q,
)

@router.post(
Expand Down
3 changes: 1 addition & 2 deletions lib/galaxy/webapps/galaxy/services/libraries.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,6 @@ def get_permissions(
is_library_access: Optional[bool] = False,
page: int = 1,
page_limit: int = 10,
query: Optional[str] = None,
) -> Union[LibraryCurrentPermissions, LibraryAvailablePermissions]:
"""Load all permissions for the given library id and return it.
Expand Down Expand Up @@ -167,7 +166,7 @@ def get_permissions(
# Return roles that are available to select.
elif scope == LibraryPermissionScope.available:
roles, total_roles = trans.app.security_agent.get_valid_roles(
trans, library, query, page, page_limit, is_library_access
trans, library, page, page_limit, is_library_access
)

return_roles = []
Expand Down
3 changes: 1 addition & 2 deletions lib/galaxy/webapps/galaxy/services/library_folders.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ def get_permissions(
scope: Optional[LibraryPermissionScope] = LibraryPermissionScope.current,
page: int = 1,
page_limit: int = 10,
query: Optional[str] = None,
) -> Union[LibraryFolderCurrentPermissions, LibraryAvailablePermissions]:
"""
Load all permissions for the given folder id and return it.
Expand Down Expand Up @@ -113,7 +112,7 @@ def get_permissions(
return LibraryFolderCurrentPermissions.model_construct(**current_permissions)
# Return roles that are available to select.
elif scope == LibraryPermissionScope.available:
roles, total_roles = trans.app.security_agent.get_valid_roles(trans, folder, query, page, page_limit)
roles, total_roles = trans.app.security_agent.get_valid_roles(trans, folder, page, page_limit)
return_roles = []
for role in roles:
return_roles.append(BasicRoleModel(id=role.id, name=role.name, type=role.type))
Expand Down
19 changes: 0 additions & 19 deletions lib/galaxy_test/api/test_libraries.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,25 +170,6 @@ def test_get_library_available_permissions(self):
available_role_ids = [role["id"] for role in available["roles"]]
assert role_id in available_role_ids

@requires_new_library
def test_get_library_available_permissions_with_query(self):
library = self.library_populator.new_library("GetAvailablePermissionWithQueryTestLibrary")
library_id = library["id"]

with self._different_user():
email = self.library_populator.user_email()

# test at least 2 user roles should be available now
available = self.library_populator.get_permissions(library_id, scope="available")
available_role_ids = [role["id"] for role in available["roles"]]
assert len(available_role_ids) > 1

# test query for specific role/email
available = self.library_populator.get_permissions(library_id, scope="available", q=email)
available_role_emails = [role["name"] for role in available["roles"]]
assert available["total"] == 1
assert available_role_emails[0] == email

@requires_new_library
def test_create_library_dataset_bootstrap_user(self, library_name="private_dataset", wait=True):
library = self.library_populator.new_private_library(library_name)
Expand Down

0 comments on commit 6f9fe64

Please sign in to comment.