From 32d83bc4de994e5505324a0d1eca666a8ec3e260 Mon Sep 17 00:00:00 2001 From: Arash Date: Tue, 19 Nov 2024 15:24:48 +0100 Subject: [PATCH 01/12] Enhance filename handling in data download and zipstream headers to support UTF-8 encoding --- lib/galaxy/datatypes/data.py | 21 ++++++++++++++------- lib/galaxy/util/zipstream.py | 5 ++++- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/lib/galaxy/datatypes/data.py b/lib/galaxy/datatypes/data.py index 23451220ca58..a88f0d229896 100644 --- a/lib/galaxy/datatypes/data.py +++ b/lib/galaxy/datatypes/data.py @@ -19,6 +19,7 @@ TYPE_CHECKING, Union, ) +from urllib.parse import quote from markupsafe import escape from typing_extensions import Literal @@ -430,14 +431,16 @@ def _serve_raw( headers["content-type"] = ( "application/octet-stream" # force octet-stream so Safari doesn't append mime extensions to filename ) - filename = self._download_filename( + filename, utf8_encoded_filename = self._download_filename( dataset, to_ext, hdca=kwd.get("hdca"), element_identifier=kwd.get("element_identifier"), filename_pattern=kwd.get("filename_pattern"), ) - headers["Content-Disposition"] = f'attachment; filename="{filename}"' + headers["Content-Disposition"] = ( + f"attachment; filename=\"{filename}\"; filename*=UTF-8''{utf8_encoded_filename}" + ) return open(dataset.get_file_name(), mode="rb"), headers def to_archive(self, dataset: DatasetProtocol, name: str = "") -> Iterable: @@ -473,7 +476,7 @@ def _serve_file_download(self, headers, data, trans, to_ext, file_size, **kwd): return self._archive_composite_dataset(trans, data, headers, do_action=kwd.get("do_action", "zip")) else: headers["Content-Length"] = str(file_size) - filename = self._download_filename( + filename, utf8_encoded_filename = self._download_filename( data, to_ext, hdca=kwd.get("hdca"), @@ -483,7 +486,9 @@ def _serve_file_download(self, headers, data, trans, to_ext, file_size, **kwd): headers["content-type"] = ( "application/octet-stream" # force octet-stream so Safari doesn't append mime extensions to filename ) - headers["Content-Disposition"] = f'attachment; filename="{filename}"' + headers["Content-Disposition"] = ( + f"attachment; filename=\"{filename}\"; filename*=UTF-8''{utf8_encoded_filename}" + ) return open(data.get_file_name(), "rb"), headers def _serve_binary_file_contents_as_text(self, trans, data, headers, file_size, max_peek_size): @@ -659,7 +664,7 @@ def _download_filename( hdca: Optional[DatasetHasHidProtocol] = None, element_identifier: Optional[str] = None, filename_pattern: Optional[str] = None, - ) -> str: + ) -> Tuple[str, str]: def escape(raw_identifier): return "".join(c in FILENAME_VALID_CHARS and c or "_" for c in raw_identifier)[0:150] @@ -685,8 +690,10 @@ def escape(raw_identifier): template_values["element_identifier"] = element_identifier template_values["hdca_name"] = escape(hdca.name) template_values["hdca_hid"] = hdca.hid - - return string.Template(filename_pattern).substitute(**template_values) + filename = string.Template(filename_pattern).substitute(**template_values) + template_values["name"] = quote(dataset.name, safe="") + utf8_encoded_filename = string.Template(filename_pattern).substitute(**template_values) + return filename, utf8_encoded_filename def display_name(self, dataset: HasName) -> str: """Returns formatted html of dataset name""" diff --git a/lib/galaxy/util/zipstream.py b/lib/galaxy/util/zipstream.py index 1cd1c77649e7..3ab85ac19666 100644 --- a/lib/galaxy/util/zipstream.py +++ b/lib/galaxy/util/zipstream.py @@ -42,7 +42,10 @@ def get_headers(self) -> Dict[str, str]: headers = {} if self.archive_name: archive_name = self.archive_name.encode("latin-1", "replace").decode("latin-1") - headers["Content-Disposition"] = f'attachment; filename="{archive_name}.zip"' + utf8_encoded_filename = quote(self.archive_name, safe="") + headers["Content-Disposition"] = ( + f"attachment; filename=\"{archive_name}.zip\"; filename*=UTF-8''{utf8_encoded_filename}.zip" + ) if self.upstream_mod_zip: headers["X-Archive-Files"] = "zip" else: From 4bc21f885eceea3606302a6a49a603535b04b464 Mon Sep 17 00:00:00 2001 From: Arash Date: Tue, 19 Nov 2024 16:11:29 +0100 Subject: [PATCH 02/12] Add tests for datasets and collections download with non english characters --- lib/galaxy_test/api/test_dataset_collections.py | 2 ++ lib/galaxy_test/api/test_datasets.py | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/lib/galaxy_test/api/test_dataset_collections.py b/lib/galaxy_test/api/test_dataset_collections.py index c9428ad84ffe..d7710c57b2fa 100644 --- a/lib/galaxy_test/api/test_dataset_collections.py +++ b/lib/galaxy_test/api/test_dataset_collections.py @@ -1,6 +1,7 @@ import zipfile from io import BytesIO from typing import List +from urllib.parse import quote from galaxy.util.unittest_utils import skip_if_github_down from galaxy_test.base.api_asserts import assert_object_id_error @@ -189,6 +190,7 @@ def test_download_non_english_characters(self): hdca_id = self.dataset_populator.fetch(payload, wait=True).json()["outputs"][0]["id"] create_response = self._download_dataset_collection(history_id=history_id, hdca_id=hdca_id) self._assert_status_code_is(create_response, 200) + assert quote(name, safe="") in create_response.headers["Content-Disposition"] @requires_new_user def test_hda_security(self): diff --git a/lib/galaxy_test/api/test_datasets.py b/lib/galaxy_test/api/test_datasets.py index dd9084c8c4f4..87ec35daf517 100644 --- a/lib/galaxy_test/api/test_datasets.py +++ b/lib/galaxy_test/api/test_datasets.py @@ -6,6 +6,7 @@ Dict, List, ) +from urllib.parse import quote from galaxy.model.unittest_utils.store_fixtures import ( deferred_hda_model_store_dict, @@ -897,3 +898,10 @@ def test_cannot_update_datatype_on_immutable_history(self, history_id): response = self._put(f"histories/{history_id}/contents/{hda_id}", data={"datatype": "tabular"}, json=True) self._assert_status_code_is(response, 403) assert response.json()["err_msg"] == "History is immutable" + + def test_download_non_english_characters(self, history_id): + name = "دیتاست" + hda = self.dataset_populator.new_dataset(history_id=history_id, name=name, content="data", wait=True) + response = self._get(f"histories/{history_id}/contents/{hda['id']}/display?to_ext=json") + self._assert_status_code_is(response, 200) + assert quote(name, safe="") in response.headers["Content-Disposition"] From 2473db082aaf6316764170bfd9e1ec332f71ea08 Mon Sep 17 00:00:00 2001 From: John Davis Date: Wed, 20 Nov 2024 00:47:57 -0500 Subject: [PATCH 03/12] Update version to 24.3.dev0 --- Makefile | 2 +- lib/galaxy/version.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index fbfd14558e1d..e00ed104e8f2 100644 --- a/Makefile +++ b/Makefile @@ -2,7 +2,7 @@ VENV?=.venv # Source virtualenv to execute command (darker, sphinx, twine, etc...) IN_VENV=if [ -f "$(VENV)/bin/activate" ]; then . "$(VENV)/bin/activate"; fi; -RELEASE_CURR:=24.2 +RELEASE_CURR:=24.3 RELEASE_UPSTREAM:=upstream CONFIG_MANAGE=$(IN_VENV) python lib/galaxy/config/config_manage.py PROJECT_URL?=https://github.com/galaxyproject/galaxy diff --git a/lib/galaxy/version.py b/lib/galaxy/version.py index 319c7fefb8a0..901930688384 100644 --- a/lib/galaxy/version.py +++ b/lib/galaxy/version.py @@ -1,3 +1,3 @@ -VERSION_MAJOR = "24.2" +VERSION_MAJOR = "24.3" VERSION_MINOR = "dev0" VERSION = VERSION_MAJOR + (f".{VERSION_MINOR}" if VERSION_MINOR else "") From 18a0b927333b42a882d2bea37682f8e6e188a071 Mon Sep 17 00:00:00 2001 From: Arash Date: Wed, 20 Nov 2024 11:11:00 +0100 Subject: [PATCH 04/12] Refactor filename handling in data download to use a dedicated toContentDisposition function for improved UTF-8 support --- lib/galaxy/datatypes/data.py | 32 ++++++++++++-------------------- lib/galaxy/util/__init__.py | 6 ++++++ lib/galaxy/util/zipstream.py | 7 ++----- 3 files changed, 20 insertions(+), 25 deletions(-) diff --git a/lib/galaxy/datatypes/data.py b/lib/galaxy/datatypes/data.py index a88f0d229896..0677498f68c4 100644 --- a/lib/galaxy/datatypes/data.py +++ b/lib/galaxy/datatypes/data.py @@ -19,7 +19,6 @@ TYPE_CHECKING, Union, ) -from urllib.parse import quote from markupsafe import escape from typing_extensions import Literal @@ -51,6 +50,7 @@ FILENAME_VALID_CHARS, inflector, iter_start_of_line, + toContentDisposition, unicodify, UNKNOWN, ) @@ -431,16 +431,14 @@ def _serve_raw( headers["content-type"] = ( "application/octet-stream" # force octet-stream so Safari doesn't append mime extensions to filename ) - filename, utf8_encoded_filename = self._download_filename( + filename = self._download_filename( dataset, to_ext, hdca=kwd.get("hdca"), element_identifier=kwd.get("element_identifier"), filename_pattern=kwd.get("filename_pattern"), ) - headers["Content-Disposition"] = ( - f"attachment; filename=\"{filename}\"; filename*=UTF-8''{utf8_encoded_filename}" - ) + headers["Content-Disposition"] = toContentDisposition(filename) return open(dataset.get_file_name(), mode="rb"), headers def to_archive(self, dataset: DatasetProtocol, name: str = "") -> Iterable: @@ -476,7 +474,7 @@ def _serve_file_download(self, headers, data, trans, to_ext, file_size, **kwd): return self._archive_composite_dataset(trans, data, headers, do_action=kwd.get("do_action", "zip")) else: headers["Content-Length"] = str(file_size) - filename, utf8_encoded_filename = self._download_filename( + filename = self._download_filename( data, to_ext, hdca=kwd.get("hdca"), @@ -486,9 +484,7 @@ def _serve_file_download(self, headers, data, trans, to_ext, file_size, **kwd): headers["content-type"] = ( "application/octet-stream" # force octet-stream so Safari doesn't append mime extensions to filename ) - headers["Content-Disposition"] = ( - f"attachment; filename=\"{filename}\"; filename*=UTF-8''{utf8_encoded_filename}" - ) + headers["Content-Disposition"] = toContentDisposition(filename) return open(data.get_file_name(), "rb"), headers def _serve_binary_file_contents_as_text(self, trans, data, headers, file_size, max_peek_size): @@ -664,17 +660,14 @@ def _download_filename( hdca: Optional[DatasetHasHidProtocol] = None, element_identifier: Optional[str] = None, filename_pattern: Optional[str] = None, - ) -> Tuple[str, str]: - def escape(raw_identifier): - return "".join(c in FILENAME_VALID_CHARS and c or "_" for c in raw_identifier)[0:150] - + ) -> str: if not to_ext or to_ext == "data": # If a client requests to_ext with the extension 'data', they are # deferring to the server, set it based on datatype. to_ext = dataset.extension template_values = { - "name": escape(dataset.name), + "name": dataset.name, "ext": to_ext, "hid": dataset.hid, } @@ -687,13 +680,12 @@ def escape(raw_identifier): if hdca is not None: # Use collection context to build up filename. - template_values["element_identifier"] = element_identifier - template_values["hdca_name"] = escape(hdca.name) + if element_identifier is not None: + template_values["element_identifier"] = element_identifier + template_values["hdca_name"] = hdca.name template_values["hdca_hid"] = hdca.hid - filename = string.Template(filename_pattern).substitute(**template_values) - template_values["name"] = quote(dataset.name, safe="") - utf8_encoded_filename = string.Template(filename_pattern).substitute(**template_values) - return filename, utf8_encoded_filename + + return string.Template(filename_pattern).substitute(**template_values) def display_name(self, dataset: HasName) -> str: """Returns formatted html of dataset name""" diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py index 1b0ec302fb71..f0c1938f3021 100644 --- a/lib/galaxy/util/__init__.py +++ b/lib/galaxy/util/__init__.py @@ -49,6 +49,7 @@ Union, ) from urllib.parse import ( + quote, urlencode, urlparse, urlsplit, @@ -2006,3 +2007,8 @@ def lowercase_alphanum_to_hex(lowercase_alphanum: str) -> str: import numpy as np return np.base_repr(int(lowercase_alphanum, 36), 16).lower() + + +def toContentDisposition(filename: str) -> str: + utf8_encoded_filename = quote(filename, safe="") + return f"attachment; filename=\"{utf8_encoded_filename}\"; filename*=UTF-8''{utf8_encoded_filename}" diff --git a/lib/galaxy/util/zipstream.py b/lib/galaxy/util/zipstream.py index 3ab85ac19666..8a643d03d3e1 100644 --- a/lib/galaxy/util/zipstream.py +++ b/lib/galaxy/util/zipstream.py @@ -11,6 +11,7 @@ import zipstream +from galaxy.util import toContentDisposition from .path import safe_walk CRC32_MIN = 1444 @@ -41,11 +42,7 @@ def response(self) -> Iterator[bytes]: def get_headers(self) -> Dict[str, str]: headers = {} if self.archive_name: - archive_name = self.archive_name.encode("latin-1", "replace").decode("latin-1") - utf8_encoded_filename = quote(self.archive_name, safe="") - headers["Content-Disposition"] = ( - f"attachment; filename=\"{archive_name}.zip\"; filename*=UTF-8''{utf8_encoded_filename}.zip" - ) + headers["Content-Disposition"] = toContentDisposition(f"{self.archive_name}.zip") if self.upstream_mod_zip: headers["X-Archive-Files"] = "zip" else: From 1388ea151f8539e143c084dde8b668960ebde356 Mon Sep 17 00:00:00 2001 From: Arash Date: Wed, 20 Nov 2024 13:01:59 +0100 Subject: [PATCH 05/12] Sanitize filenames in toContentDisposition function to ensure valid characters are used --- lib/galaxy/util/__init__.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py index f0c1938f3021..e8ce53de1b61 100644 --- a/lib/galaxy/util/__init__.py +++ b/lib/galaxy/util/__init__.py @@ -2010,5 +2010,6 @@ def lowercase_alphanum_to_hex(lowercase_alphanum: str) -> str: def toContentDisposition(filename: str) -> str: + sanitized_filename = "".join(c in FILENAME_VALID_CHARS and c or "_" for c in filename)[0:150] utf8_encoded_filename = quote(filename, safe="") - return f"attachment; filename=\"{utf8_encoded_filename}\"; filename*=UTF-8''{utf8_encoded_filename}" + return f"attachment; filename=\"{sanitized_filename}\"; filename*=UTF-8''{utf8_encoded_filename}" From 77767b3dc19bcea31047236f36a203a0f8c82dc5 Mon Sep 17 00:00:00 2001 From: Arash Date: Wed, 20 Nov 2024 14:40:29 +0100 Subject: [PATCH 06/12] Rename toContentDisposition function to to_content_disposition apply 255 characters limit to filename exclude `/\?%*:|"<>` characters from encoded filename --- lib/galaxy/datatypes/data.py | 6 +++--- lib/galaxy/util/__init__.py | 7 ++++--- lib/galaxy/util/zipstream.py | 4 ++-- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/galaxy/datatypes/data.py b/lib/galaxy/datatypes/data.py index 0677498f68c4..1cf74989976a 100644 --- a/lib/galaxy/datatypes/data.py +++ b/lib/galaxy/datatypes/data.py @@ -50,7 +50,7 @@ FILENAME_VALID_CHARS, inflector, iter_start_of_line, - toContentDisposition, + to_content_disposition, unicodify, UNKNOWN, ) @@ -438,7 +438,7 @@ def _serve_raw( element_identifier=kwd.get("element_identifier"), filename_pattern=kwd.get("filename_pattern"), ) - headers["Content-Disposition"] = toContentDisposition(filename) + headers["Content-Disposition"] = to_content_disposition(filename) return open(dataset.get_file_name(), mode="rb"), headers def to_archive(self, dataset: DatasetProtocol, name: str = "") -> Iterable: @@ -484,7 +484,7 @@ def _serve_file_download(self, headers, data, trans, to_ext, file_size, **kwd): headers["content-type"] = ( "application/octet-stream" # force octet-stream so Safari doesn't append mime extensions to filename ) - headers["Content-Disposition"] = toContentDisposition(filename) + headers["Content-Disposition"] = to_content_disposition(filename) return open(data.get_file_name(), "rb"), headers def _serve_binary_file_contents_as_text(self, trans, data, headers, file_size, max_peek_size): diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py index e8ce53de1b61..9b3044fb429f 100644 --- a/lib/galaxy/util/__init__.py +++ b/lib/galaxy/util/__init__.py @@ -2009,7 +2009,8 @@ def lowercase_alphanum_to_hex(lowercase_alphanum: str) -> str: return np.base_repr(int(lowercase_alphanum, 36), 16).lower() -def toContentDisposition(filename: str) -> str: - sanitized_filename = "".join(c in FILENAME_VALID_CHARS and c or "_" for c in filename)[0:150] - utf8_encoded_filename = quote(filename, safe="") +def to_content_disposition(target: str) -> str: + filename, ext = os.path.splitext(target) + sanitized_filename = "".join(c in FILENAME_VALID_CHARS and c or "_" for c in filename)[0:255] + ext + utf8_encoded_filename = quote(re.sub(r'[\/\\\?%*:|"<>]', "_", filename), safe="")[0:255] + ext return f"attachment; filename=\"{sanitized_filename}\"; filename*=UTF-8''{utf8_encoded_filename}" diff --git a/lib/galaxy/util/zipstream.py b/lib/galaxy/util/zipstream.py index 8a643d03d3e1..7b81d1c33f4b 100644 --- a/lib/galaxy/util/zipstream.py +++ b/lib/galaxy/util/zipstream.py @@ -11,7 +11,7 @@ import zipstream -from galaxy.util import toContentDisposition +from galaxy.util import to_content_disposition from .path import safe_walk CRC32_MIN = 1444 @@ -42,7 +42,7 @@ def response(self) -> Iterator[bytes]: def get_headers(self) -> Dict[str, str]: headers = {} if self.archive_name: - headers["Content-Disposition"] = toContentDisposition(f"{self.archive_name}.zip") + headers["Content-Disposition"] = to_content_disposition(f"{self.archive_name}.zip") if self.upstream_mod_zip: headers["X-Archive-Files"] = "zip" else: From dfafd7a8beb2ebf68c29017d24eaf26279548f68 Mon Sep 17 00:00:00 2001 From: Arash Date: Wed, 20 Nov 2024 14:46:53 +0100 Subject: [PATCH 07/12] exclude ext from character_limit --- lib/galaxy/util/__init__.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py index 9b3044fb429f..d2c8617c08bd 100644 --- a/lib/galaxy/util/__init__.py +++ b/lib/galaxy/util/__init__.py @@ -2011,6 +2011,7 @@ def lowercase_alphanum_to_hex(lowercase_alphanum: str) -> str: def to_content_disposition(target: str) -> str: filename, ext = os.path.splitext(target) - sanitized_filename = "".join(c in FILENAME_VALID_CHARS and c or "_" for c in filename)[0:255] + ext - utf8_encoded_filename = quote(re.sub(r'[\/\\\?%*:|"<>]', "_", filename), safe="")[0:255] + ext + character_limit = 255 - len(ext) + sanitized_filename = "".join(c in FILENAME_VALID_CHARS and c or "_" for c in filename)[0:character_limit] + ext + utf8_encoded_filename = quote(re.sub(r'[\/\\\?%*:|"<>]', "_", filename), safe="")[0:character_limit] + ext return f"attachment; filename=\"{sanitized_filename}\"; filename*=UTF-8''{utf8_encoded_filename}" From 1cdde377fbb76e7792e43309fabd2f49afdeda65 Mon Sep 17 00:00:00 2001 From: John Davis Date: Wed, 20 Nov 2024 09:34:22 -0500 Subject: [PATCH 08/12] Fix version: 25.0 --- Makefile | 2 +- lib/galaxy/version.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index e00ed104e8f2..64d649ebf188 100644 --- a/Makefile +++ b/Makefile @@ -2,7 +2,7 @@ VENV?=.venv # Source virtualenv to execute command (darker, sphinx, twine, etc...) IN_VENV=if [ -f "$(VENV)/bin/activate" ]; then . "$(VENV)/bin/activate"; fi; -RELEASE_CURR:=24.3 +RELEASE_CURR:=25.0 RELEASE_UPSTREAM:=upstream CONFIG_MANAGE=$(IN_VENV) python lib/galaxy/config/config_manage.py PROJECT_URL?=https://github.com/galaxyproject/galaxy diff --git a/lib/galaxy/version.py b/lib/galaxy/version.py index 901930688384..83b664d1d26f 100644 --- a/lib/galaxy/version.py +++ b/lib/galaxy/version.py @@ -1,3 +1,3 @@ -VERSION_MAJOR = "24.3" +VERSION_MAJOR = "25.0" VERSION_MINOR = "dev0" VERSION = VERSION_MAJOR + (f".{VERSION_MINOR}" if VERSION_MINOR else "") From b3e57cb9a2cf7e4bb856c368270a935cb9c1e575 Mon Sep 17 00:00:00 2001 From: John Davis Date: Thu, 31 Oct 2024 18:08:50 -0400 Subject: [PATCH 09/12] Cleanup sharing roles on user purge, refactor, test 1. Update sharing role name removing purged user email 2. If sharing role has only one user association, delete it. 3. Factor out cleanup, add test. Note: this will not work if user email has been udpated after the sharing role was created. --- lib/galaxy/managers/users.py | 29 ++++++++-- test/unit/data/model/db/test_user.py | 81 ++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 4 deletions(-) diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py index fc8644d1ba4e..53173519d088 100644 --- a/lib/galaxy/managers/users.py +++ b/lib/galaxy/managers/users.py @@ -40,6 +40,7 @@ from galaxy.managers.base import combine_lists from galaxy.model import ( Job, + Role, User, UserAddress, UserQuotaUsage, @@ -214,10 +215,7 @@ def purge(self, user, flush=True): # Delete UserGroupAssociations for uga in user.groups: self.session().delete(uga) - # Delete UserRoleAssociations EXCEPT FOR THE PRIVATE ROLE - for ura in user.roles: - if ura.role_id != private_role.id: - self.session().delete(ura) + _cleanup_nonprivate_user_roles(self.session(), user, private_role.id) # Delete UserAddresses for address in user.addresses: self.session().delete(address) @@ -891,3 +889,26 @@ def generate_next_available_username(session, username, model_class=User): while session.execute(select(model_class).where(model_class.username == f"{username}-{i}")).first(): i += 1 return f"{username}-{i}" + + +def _cleanup_nonprivate_user_roles(session, user, private_role_id): + """ + Delete UserRoleAssociations EXCEPT FOR THE PRIVATE ROLE; + Delete sharing roles that are associated with this user only; + Remove user email from sharing role names associated with multiple users. + + Note: this method updates the session without flushing or committing. + """ + user_roles = [ura for ura in user.roles if ura.role_id != private_role_id] + for user_role_assoc in user_roles: + role = user_role_assoc.role + if role.type == Role.types.SHARING: + if len(role.users) == 1: + # This role is associated with this user only, so we can delete it + session.delete(role) + elif user.email in role.name: + # Remove user email from sharing role's name + role.name = role.name.replace(user.email, "[USER PURGED]") + session.add(role) + # Delete user role association + session.delete(user_role_assoc) diff --git a/test/unit/data/model/db/test_user.py b/test/unit/data/model/db/test_user.py index 87d136a125a4..3a4d28286562 100644 --- a/test/unit/data/model/db/test_user.py +++ b/test/unit/data/model/db/test_user.py @@ -1,6 +1,12 @@ import pytest +from sqlalchemy import select from sqlalchemy.exc import IntegrityError +from galaxy.managers.users import _cleanup_nonprivate_user_roles +from galaxy.model import ( + Role, + UserRoleAssociation, +) from galaxy.model.db.user import ( get_user_by_email, get_user_by_username, @@ -80,3 +86,78 @@ def test_username_is_unique(make_user): make_user(username="a") with pytest.raises(IntegrityError): make_user(username="a") + + +def test_cleanup_nonprivate_user_roles(session, make_user_and_role, make_role, make_user_role_association): + # Create 3 users with private roles + user1, role_private1 = make_user_and_role(email="user1@foo.com") + user2, role_private2 = make_user_and_role(email="user2@foo.com") + user3, role_private3 = make_user_and_role(email="user3@foo.com") + + # Create role_sharing1 and associated it with user1 and user2 + role_sharing1 = make_role(type=Role.types.SHARING, name="sharing role for user1@foo.com, user2@foo.com") + make_user_role_association(user1, role_sharing1) + make_user_role_association(user2, role_sharing1) + + # Create role_sharing2 and associated it with user3 + role_sharing2 = make_role(type=Role.types.SHARING, name="sharing role for user3@foo.com") + make_user_role_association(user3, role_sharing2) + + # Create another role and associate it with all users + role6 = make_role() + make_user_role_association(user1, role6) + make_user_role_association(user2, role6) + make_user_role_association(user3, role6) + + # verify number of all user role associations + associations = session.scalars(select(UserRoleAssociation)).all() + assert len(associations) == 9 # 3 private, 2 + 1 sharing, 3 with role6 + + # verify user1 roles + assert len(user1.roles) == 3 + assert role_private1 in [ura.role for ura in user1.roles] + assert role_sharing1 in [ura.role for ura in user1.roles] + assert role6 in [ura.role for ura in user1.roles] + + # run cleanup on user user1 + _cleanup_nonprivate_user_roles(session, user1, role_private1.id) + session.commit() # must commit since method does not commit + + # private role not deleted, associations with role6 and with sharing role deleted, sharing role name updated + assert len(user1.roles) == 1 + assert user1.roles[0].id == role_private1.id + assert role_sharing1.name == "sharing role for [USER PURGED], user2@foo.com" + + # verify user2 roles + assert len(user2.roles) == 3 + assert role_private2 in [ura.role for ura in user2.roles] + assert role_sharing1 in [ura.role for ura in user2.roles] + assert role6 in [ura.role for ura in user2.roles] + + # run cleanup on user user2 + _cleanup_nonprivate_user_roles(session, user2, role_private2.id) + session.commit() + + # private role not deleted, association with sharing role deleted + assert len(user2.roles) == 1 + assert user2.roles[0].id == role_private2.id + # sharing role1 deleted since it has no more users associated with it + roles = session.scalars(select(Role)).all() + assert len(roles) == 5 # 3 private, role6, sharing2 + assert role_sharing1.id not in [role.id for role in roles] + + # verify user3 roles + assert len(user3.roles) == 3 + assert role_private3 in [ura.role for ura in user3.roles] + assert role_sharing2 in [ura.role for ura in user3.roles] + assert role6 in [ura.role for ura in user3.roles] + + # run cleanup on user user3 + _cleanup_nonprivate_user_roles(session, user3, role_private3.id) + session.commit() + + # remaining: 3 private roles + 3 associations, role6 + roles = session.scalars(select(Role)).all() + assert len(roles) == 4 + associations = session.scalars(select(UserRoleAssociation)).all() + assert len(associations) == 3 From 16f7700ea4d0bb4a1d6ff26e19b04cfaa00f69b2 Mon Sep 17 00:00:00 2001 From: John Davis Date: Fri, 1 Nov 2024 00:37:57 -0400 Subject: [PATCH 10/12] Move method out of managers into model To accommodate package unit testing --- lib/galaxy/managers/users.py | 25 +------------------------ lib/galaxy/model/db/user.py | 28 +++++++++++++++++++++++++++- test/unit/data/model/db/test_user.py | 2 +- 3 files changed, 29 insertions(+), 26 deletions(-) diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py index 53173519d088..d1f24e4cb0b6 100644 --- a/lib/galaxy/managers/users.py +++ b/lib/galaxy/managers/users.py @@ -40,13 +40,13 @@ from galaxy.managers.base import combine_lists from galaxy.model import ( Job, - Role, User, UserAddress, UserQuotaUsage, ) from galaxy.model.base import transaction from galaxy.model.db.user import ( + _cleanup_nonprivate_user_roles, get_user_by_email, get_user_by_username, ) @@ -889,26 +889,3 @@ def generate_next_available_username(session, username, model_class=User): while session.execute(select(model_class).where(model_class.username == f"{username}-{i}")).first(): i += 1 return f"{username}-{i}" - - -def _cleanup_nonprivate_user_roles(session, user, private_role_id): - """ - Delete UserRoleAssociations EXCEPT FOR THE PRIVATE ROLE; - Delete sharing roles that are associated with this user only; - Remove user email from sharing role names associated with multiple users. - - Note: this method updates the session without flushing or committing. - """ - user_roles = [ura for ura in user.roles if ura.role_id != private_role_id] - for user_role_assoc in user_roles: - role = user_role_assoc.role - if role.type == Role.types.SHARING: - if len(role.users) == 1: - # This role is associated with this user only, so we can delete it - session.delete(role) - elif user.email in role.name: - # Remove user email from sharing role's name - role.name = role.name.replace(user.email, "[USER PURGED]") - session.add(role) - # Delete user role association - session.delete(user_role_assoc) diff --git a/lib/galaxy/model/db/user.py b/lib/galaxy/model/db/user.py index 823247f725dd..00f824f5e1ee 100644 --- a/lib/galaxy/model/db/user.py +++ b/lib/galaxy/model/db/user.py @@ -8,7 +8,10 @@ true, ) -from galaxy.model import User +from galaxy.model import ( + Role, + User, +) from galaxy.model.scoped_session import galaxy_scoped_session @@ -64,3 +67,26 @@ def get_users_for_index( else: stmt = stmt.where(User.deleted == false()) return session.scalars(stmt).all() + + +def _cleanup_nonprivate_user_roles(session, user, private_role_id): + """ + Delete UserRoleAssociations EXCEPT FOR THE PRIVATE ROLE; + Delete sharing roles that are associated with this user only; + Remove user email from sharing role names associated with multiple users. + + Note: this method updates the session without flushing or committing. + """ + user_roles = [ura for ura in user.roles if ura.role_id != private_role_id] + for user_role_assoc in user_roles: + role = user_role_assoc.role + if role.type == Role.types.SHARING: + if len(role.users) == 1: + # This role is associated with this user only, so we can delete it + session.delete(role) + elif user.email in role.name: + # Remove user email from sharing role's name + role.name = role.name.replace(user.email, "[USER PURGED]") + session.add(role) + # Delete user role association + session.delete(user_role_assoc) diff --git a/test/unit/data/model/db/test_user.py b/test/unit/data/model/db/test_user.py index 3a4d28286562..042a541eb817 100644 --- a/test/unit/data/model/db/test_user.py +++ b/test/unit/data/model/db/test_user.py @@ -2,12 +2,12 @@ from sqlalchemy import select from sqlalchemy.exc import IntegrityError -from galaxy.managers.users import _cleanup_nonprivate_user_roles from galaxy.model import ( Role, UserRoleAssociation, ) from galaxy.model.db.user import ( + _cleanup_nonprivate_user_roles, get_user_by_email, get_user_by_username, get_users_by_ids, From 725c120428c0fa2e29be9d0c694b80f32963f294 Mon Sep 17 00:00:00 2001 From: John Davis Date: Fri, 1 Nov 2024 21:52:34 -0400 Subject: [PATCH 11/12] Cleanup wording, left over debug code --- lib/galaxy/webapps/galaxy/api/users.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/webapps/galaxy/api/users.py b/lib/galaxy/webapps/galaxy/api/users.py index c9d4f8bd50ee..51b5dcea8ebd 100644 --- a/lib/galaxy/webapps/galaxy/api/users.py +++ b/lib/galaxy/webapps/galaxy/api/users.py @@ -734,7 +734,7 @@ def 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.", + description="Return a list of roles associated with this user. Only admins can see user roles.", require_admin=True, ) def get_user_roles( From df850ac7c86961b50f3c93890c6d5083bc593c78 Mon Sep 17 00:00:00 2001 From: John Davis Date: Sat, 2 Nov 2024 18:07:01 -0400 Subject: [PATCH 12/12] Rebuild schema --- client/src/api/schema/schema.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/src/api/schema/schema.ts b/client/src/api/schema/schema.ts index 7da851a70511..abc681fb8ca3 100644 --- a/client/src/api/schema/schema.ts +++ b/client/src/api/schema/schema.ts @@ -4804,7 +4804,7 @@ export interface paths { }; /** * Get User Roles - * @description Return a collection of roles associated with this user. Only admins can see user roles. + * @description Return a list 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;