From 6af4138547f15c9438ba13c073bc8e1fad3cd63d Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 4 Jun 2024 18:01:02 +0200 Subject: [PATCH 1/5] Skip new history creation of user is anonymous and login is required Most straightforward fix for https://github.com/galaxyproject/galaxy/issues/18317. I have not been able to reproduce the issue itself though, only a single history is being created in my testing. --- lib/galaxy/webapps/base/webapp.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/galaxy/webapps/base/webapp.py b/lib/galaxy/webapps/base/webapp.py index b9430a3774c0..b9bd9eb717ce 100644 --- a/lib/galaxy/webapps/base/webapp.py +++ b/lib/galaxy/webapps/base/webapp.py @@ -932,6 +932,10 @@ def get_or_create_default_history(self): self.set_history(history) return history + # Don't create new history if login required and user is anonymous + if self.app.config.require_login and not self.user: + return None + # No suitable history found, create a new one. return self.new_history() From 57f524706063b5d093da5b5581360f9003bb6e2d Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 5 Jun 2024 14:42:07 +0200 Subject: [PATCH 2/5] Add integration tests for exposing user information --- test/integration/test_users.py | 91 ++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 test/integration/test_users.py diff --git a/test/integration/test_users.py b/test/integration/test_users.py new file mode 100644 index 000000000000..1b3b6714984c --- /dev/null +++ b/test/integration/test_users.py @@ -0,0 +1,91 @@ +from typing import ClassVar + +from galaxy_test.driver import integration_util + +USER_SUMMARY_KEYS = set(["model_class", "id", "email", "username", "deleted", "active", "last_password_change"]) + + +class UsersIntegrationTestCase(integration_util.IntegrationTestCase): + expose_user_name: ClassVar[bool] + expose_user_email: ClassVar[bool] + expected_regular_user_list_count: ClassVar[int] + expected_limited_user_keys: ClassVar[set] + + @classmethod + def handle_galaxy_config_kwds(cls, config): + super().handle_galaxy_config_kwds(config) + config["expose_user_name"] = cls.expose_user_name + config["expose_user_email"] = cls.expose_user_email + + def setUp(self): + super().setUp() + self._setup_users() + + def _setup_users(self): + self.user = self._get("users/current").json() + self.user2 = self._setup_user("user02@test.gx") + self.user3 = self._setup_user("user03@test.gx") + + def test_admin_index(self): + all_users_response = self._get("users", admin=True) + self._assert_status_code_is(all_users_response, 200) + all_users = all_users_response.json() + assert len(all_users) == 3 + for user in all_users: + self._assert_has_keys(user, *USER_SUMMARY_KEYS) + + def test_user_index(self): + requesting_user_id = self.user["id"] + all_users_response = self._get("users") + self._assert_status_code_is(all_users_response, 200) + all_users = all_users_response.json() + assert len(all_users) == self.expected_regular_user_list_count + + unexpected_user_keys = USER_SUMMARY_KEYS - self.expected_limited_user_keys + for user in all_users: + if user["id"] == requesting_user_id: + # Requesting users should be able to see their own information. + self._assert_has_keys(user, *USER_SUMMARY_KEYS) + continue + # The user should be able to see other users information depending on the configuration. + self._assert_has_keys(user, *self.expected_limited_user_keys) + self._assert_not_has_keys(user, *unexpected_user_keys) + + +class TestExposeUsersIntegration(UsersIntegrationTestCase): + expose_user_name = True + expose_user_email = True + + # Since we allow to expose user information, all users are returned. + expected_limited_user_keys = set(["id", "username", "email"]) + expected_regular_user_list_count = 3 + + +class TestExposeOnlyUserNameIntegration(UsersIntegrationTestCase): + expose_user_name = True + expose_user_email = False + + # When only username is exposed, only that field is returned in the user list. + # Since we are exposing user information, all users are returned. + expected_limited_user_keys = set(["id", "username"]) + expected_regular_user_list_count = 3 + + +class TestExposeOnlyUserEmailIntegration(UsersIntegrationTestCase): + expose_user_name = False + expose_user_email = True + + # When only email is exposed, only that field is returned in the user list. + # Since we are exposing user information, all users are returned. + expected_limited_user_keys = set(["id", "email"]) + expected_regular_user_list_count = 3 + + +class TestUnexposedUsersIntegration(UsersIntegrationTestCase): + expose_user_name = False + expose_user_email = False + + # Since no user information is exposed, only the current user should be returned. + # And the current user has all fields, so no limited fields. + expected_limited_user_keys = set() + expected_regular_user_list_count = 1 From 408aa064c98529be74adf666e75780460be28de4 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 5 Jun 2024 14:59:51 +0200 Subject: [PATCH 3/5] Fix user model serialization when listing users --- lib/galaxy/schema/schema.py | 3 +++ lib/galaxy/webapps/galaxy/api/users.py | 7 +++---- lib/galaxy/webapps/galaxy/services/users.py | 23 +++++++++++---------- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/lib/galaxy/schema/schema.py b/lib/galaxy/schema/schema.py index 2e3ef4b5f9fc..c624bab55df5 100644 --- a/lib/galaxy/schema/schema.py +++ b/lib/galaxy/schema/schema.py @@ -349,6 +349,9 @@ class LimitedUserModel(Model): email: Optional[str] = None +MaybeLimitedUserModel = Union[UserModel, LimitedUserModel] + + class DiskUsageUserModel(Model): total_disk_usage: float = TotalDiskUsageField nice_total_disk_usage: str = NiceTotalDiskUsageField diff --git a/lib/galaxy/webapps/galaxy/api/users.py b/lib/galaxy/webapps/galaxy/api/users.py index 91102757c751..f3139f93c9bb 100644 --- a/lib/galaxy/webapps/galaxy/api/users.py +++ b/lib/galaxy/webapps/galaxy/api/users.py @@ -55,12 +55,11 @@ FavoriteObjectsSummary, FavoriteObjectType, FlexibleUserIdType, - LimitedUserModel, + MaybeLimitedUserModel, RemoteUserCreationPayload, UserBeaconSetting, UserCreationPayload, UserDeletionPayload, - UserModel, ) from galaxy.security.validate_user_input import ( validate_email, @@ -202,7 +201,7 @@ def index_deleted( f_email: Optional[str] = FilterEmailQueryParam, f_name: Optional[str] = FilterNameQueryParam, f_any: Optional[str] = FilterAnyQueryParam, - ) -> List[Union[UserModel, LimitedUserModel]]: + ) -> List[MaybeLimitedUserModel]: return self.service.get_index(trans=trans, deleted=True, f_email=f_email, f_name=f_name, f_any=f_any) @router.post( @@ -634,7 +633,7 @@ def index( f_email: Optional[str] = FilterEmailQueryParam, f_name: Optional[str] = FilterNameQueryParam, f_any: Optional[str] = FilterAnyQueryParam, - ) -> List[Union[UserModel, LimitedUserModel]]: + ) -> List[MaybeLimitedUserModel]: return self.service.get_index(trans=trans, deleted=deleted, f_email=f_email, f_name=f_name, f_any=f_any) @router.get( diff --git a/lib/galaxy/webapps/galaxy/services/users.py b/lib/galaxy/webapps/galaxy/services/users.py index 909a9898f193..0317af35cacc 100644 --- a/lib/galaxy/webapps/galaxy/services/users.py +++ b/lib/galaxy/webapps/galaxy/services/users.py @@ -35,6 +35,7 @@ DetailedUserModel, FlexibleUserIdType, LimitedUserModel, + MaybeLimitedUserModel, UserModel, ) from galaxy.security.idencoding import IdEncodingHelper @@ -204,8 +205,8 @@ def get_index( f_email: Optional[str], f_name: Optional[str], f_any: Optional[str], - ) -> List[Union[UserModel, LimitedUserModel]]: - rval = [] + ) -> List[MaybeLimitedUserModel]: + rval: List[MaybeLimitedUserModel] = [] stmt = select(User) if f_email and (trans.user_is_admin or trans.app.config.expose_user_email): @@ -240,13 +241,12 @@ def get_index( and not trans.app.config.expose_user_email ): if trans.user: - item = trans.user.to_dict() - return [item] + return [UserModel(**trans.user.to_dict())] else: return [] stmt = stmt.filter(User.deleted == false()) for user in trans.sa_session.scalars(stmt).all(): - item = user.to_dict() + user_dict = user.to_dict() # If NOT configured to expose_email, do not expose email UNLESS the user is self, or # the user is an admin if user is not trans.user and not trans.user_is_admin: @@ -255,12 +255,13 @@ def get_index( expose_keys.append("username") if trans.app.config.expose_user_email: expose_keys.append("email") - new_item = {} - for key, value in item.items(): + limited_user = {} + for key, value in user_dict.items(): if key in expose_keys: - new_item[key] = value - item = new_item + limited_user[key] = value + user = LimitedUserModel(**limited_user) + else: + user = UserModel(**user_dict) - # TODO: move into api_values - rval.append(item) + rval.append(user) return rval From 62049e3803f355d61b62f7e8274862686a58be0f Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 5 Jun 2024 15:30:26 +0200 Subject: [PATCH 4/5] Rename integration test base class To make the `check_test_class_names` linter happy --- test/integration/test_users.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/integration/test_users.py b/test/integration/test_users.py index 1b3b6714984c..d47110bc96c1 100644 --- a/test/integration/test_users.py +++ b/test/integration/test_users.py @@ -5,7 +5,7 @@ USER_SUMMARY_KEYS = set(["model_class", "id", "email", "username", "deleted", "active", "last_password_change"]) -class UsersIntegrationTestCase(integration_util.IntegrationTestCase): +class UsersIntegrationCase(integration_util.IntegrationTestCase): expose_user_name: ClassVar[bool] expose_user_email: ClassVar[bool] expected_regular_user_list_count: ClassVar[int] @@ -52,7 +52,7 @@ def test_user_index(self): self._assert_not_has_keys(user, *unexpected_user_keys) -class TestExposeUsersIntegration(UsersIntegrationTestCase): +class TestExposeUsersIntegration(UsersIntegrationCase): expose_user_name = True expose_user_email = True @@ -61,7 +61,7 @@ class TestExposeUsersIntegration(UsersIntegrationTestCase): expected_regular_user_list_count = 3 -class TestExposeOnlyUserNameIntegration(UsersIntegrationTestCase): +class TestExposeOnlyUserNameIntegration(UsersIntegrationCase): expose_user_name = True expose_user_email = False @@ -71,7 +71,7 @@ class TestExposeOnlyUserNameIntegration(UsersIntegrationTestCase): expected_regular_user_list_count = 3 -class TestExposeOnlyUserEmailIntegration(UsersIntegrationTestCase): +class TestExposeOnlyUserEmailIntegration(UsersIntegrationCase): expose_user_name = False expose_user_email = True @@ -81,7 +81,7 @@ class TestExposeOnlyUserEmailIntegration(UsersIntegrationTestCase): expected_regular_user_list_count = 3 -class TestUnexposedUsersIntegration(UsersIntegrationTestCase): +class TestUnexposedUsersIntegration(UsersIntegrationCase): expose_user_name = False expose_user_email = False From 7669eff2631be77bb6fcb520cd25e8848a1ade38 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Wed, 5 Jun 2024 16:23:35 +0200 Subject: [PATCH 5/5] Downgrade doi fetch error to debug There's nothing an admin can do about, and it doesn't impact tool functionality. --- lib/galaxy/managers/citations.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/managers/citations.py b/lib/galaxy/managers/citations.py index 958617289ebb..8ae9c8c64ad7 100644 --- a/lib/galaxy/managers/citations.py +++ b/lib/galaxy/managers/citations.py @@ -150,7 +150,7 @@ def to_bibtex(self): try: self.raw_bibtex = self.doi_cache.get_bibtex(self.__doi) except Exception: - log.exception("Failed to fetch bibtex for DOI %s", self.__doi) + log.debug("Failed to fetch bibtex for DOI %s", self.__doi) if self.raw_bibtex is DoiCitation.BIBTEX_UNSET: return f"""@MISC{{{self.__doi},