From df34e9b9dbe38dfa8d7a67144e48fb0729c189f8 Mon Sep 17 00:00:00 2001 From: John Davis Date: Wed, 14 Oct 2026 13:21:44 -0400 Subject: [PATCH 1/4] Derive OIDC username from email when IdP sends no username claim Since the move to python-social-auth in 26.0, social_core's get_username falls back to a random uuid4 hex string when the IdP does not send a username claim (e.g. preferred_username, often absent with CILogon). Add a pipeline step before get_username that derives the username from the email's local part for new users, matching the pre-26.0 custos behavior. Co-Authored-By: Claude Opus 5.5 --- lib/galaxy/authnz/psa_authnz.py | 15 ++++++++++++ test/unit/authnz/test_psa_authnz.py | 36 +++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/lib/galaxy/authnz/psa_authnz.py b/lib/galaxy/authnz/psa_authnz.py index c65418e2e8c..3c0f2d925d9 100644 --- a/lib/galaxy/authnz/psa_authnz.py +++ b/lib/galaxy/authnz/psa_authnz.py @@ -119,6 +119,9 @@ AUTH_PIPELINE = ( "galaxy.authnz.psa_authnz.verify", # Checks if the current social-account is already associated in the site. "social_core.pipeline.social_auth.social_user", + # Custom Galaxy step: Fall back to the email's local part when the IdP sends + # no username claim; otherwise get_username substitutes a random string. + "galaxy.authnz.psa_authnz.username_from_email", # Make up a username for this person, appends a random string at the end if # there's any collision. "social_core.pipeline.user.get_username", @@ -707,6 +710,18 @@ def contains_required_data(response=None, is_new=False, backend=None, **kwargs): raise MalformedContents(err_msg=f"Missing refresh token. {hint_msg}") +def username_from_email(details=None, user=None, **kwargs): + """ + Pipeline step: derive a username from the email's local part for new users + whose IdP sends no username claim (e.g. no ``preferred_username``). + """ + if user or not details or details.get("username") or not details.get("email"): + return + username = ready_name_for_url(details["email"].split("@")[0]).lower() + if username: + return {"details": {**details, "username": username}} + + def verify(strategy=None, response=None, details=None, **kwargs): provider = strategy.config.get("SOCIAL_AUTH_SECONDARY_AUTH_PROVIDER") endpoint = strategy.config.get("SOCIAL_AUTH_SECONDARY_AUTH_ENDPOINT") diff --git a/test/unit/authnz/test_psa_authnz.py b/test/unit/authnz/test_psa_authnz.py index c6263274886..72676c8f626 100644 --- a/test/unit/authnz/test_psa_authnz.py +++ b/test/unit/authnz/test_psa_authnz.py @@ -26,6 +26,7 @@ from jwt import ( InvalidSignatureError, ) from social_core.backends.open_id_connect import OpenIdConnectAuth +from social_core.pipeline.user import get_username from sqlalchemy import create_engine from sqlalchemy.orm import Session @@ -36,6 +37,7 @@ from galaxy.authnz.psa_authnz import ( AUTH_PIPELINE, decode_access_token, PSAAuthnz, + username_from_email, ) @@ -367,3 +369,37 @@ def test_oidc_config_custom_auth_pipeline_and_extra(mock_oidc_config_file, mock_ app_config=mock_app.config, ) assert psa_authnz.config["SOCIAL_AUTH_PIPELINE"] == custom_auth_pipeline + tuple(custom_auth_pipeline_extra) + + +def _run_get_username(details): + backend = MagicMock() + backend.setting.side_effect = lambda name, default=None: default + strategy = MagicMock() + strategy.storage.user.username_max_length.return_value = 255 + strategy.storage.user.clean_username.side_effect = lambda value: value + strategy.storage.user.user_exists.return_value = False + return get_username(strategy, details, backend)["username"] + + +def test_username_from_email_without_username_claim(): + details = {"email": "Jane.Doe+lab@example.org", "username": None} + result = username_from_email(details=details) + assert result["details"]["username"] == "janedoelab" + assert _run_get_username(result["details"]) == "janedoelab" + + +def test_username_from_email_keeps_username_claim(): + details = {"email": "jane@example.org", "username": "jdoe"} + assert username_from_email(details=details) is None + assert _run_get_username(details) == "jdoe" + + +def test_username_from_email_skips_existing_user(): + details = {"email": "jane@example.org", "username": None} + assert username_from_email(details=details, user=MagicMock()) is None + + +def test_username_from_email_runs_before_get_username(): + assert AUTH_PIPELINE.index("galaxy.authnz.psa_authnz.username_from_email") < AUTH_PIPELINE.index( + "social_core.pipeline.user.get_username" + ) From 9cdcb8a7ab0ae57db7bdfa9cee24b15c68879259 Mon Sep 17 00:00:00 2001 From: Arash <2973722+arash77@users.noreply.github.com> Date: Thu, 15 Oct 2026 10:04:03 +0200 Subject: [PATCH 2/4] Show the error when selecting a folder fails in FilesDialog Selecting a folder lists it recursively. When the server refuses that listing, the dialog stayed on Loading, kept the folder selected and showed no error. --- .../FilesDialog/FilesDialog.test.ts | 16 +++++++++ .../components/FilesDialog/FilesDialog.vue | 36 +++++++++++-------- 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/client/src/components/FilesDialog/FilesDialog.test.ts b/client/src/components/FilesDialog/FilesDialog.test.ts index 44cc6ec7372..5fcfc628f02 100644 --- a/client/src/components/FilesDialog/FilesDialog.test.ts +++ b/client/src/components/FilesDialog/FilesDialog.test.ts @@ -261,6 +261,22 @@ describe("FilesDialog, file mode", () => { await utils.navigateBack(); expect(utils.getRenderedRows().length).toBe(rootResponse.length); }); + + it("should show the error and undo the selection when a recursive listing fails", async () => { + await utils.openRootDirectory(); + server.use( + http.get("/api/remote_files", ({ response }) => { + return response("4XX").json({ err_msg: someErrorText, err_code: 400 }, { status: 400 }); + }), + ); + + suppressDebugConsole(); // expecting error message. + + await utils.clickOn(utils.findRenderedDirectory(directoryId)); + utils.expectErrorMessage(); + expect(utils.getSelectionDialog().props("isBusy")).toBe(false); + utils.expectOkButtonDisabled(); + }); }); describe("FilesDialog, create new file source button", () => { diff --git a/client/src/components/FilesDialog/FilesDialog.vue b/client/src/components/FilesDialog/FilesDialog.vue index c608fbd727a..c3d7d06d035 100644 --- a/client/src/components/FilesDialog/FilesDialog.vue +++ b/client/src/components/FilesDialog/FilesDialog.vue @@ -182,21 +182,29 @@ function selectDirectoryRecursive(record: SelectionItem) { // look for subdirectories const recursive = true; isBusy.value = true; - browseRemoteFiles(record.url, recursive).then((incoming) => { - incoming.entries.forEach((item) => { - // construct record - const subRecord = entryToRecord(item); - if (subRecord.isLeaf) { - // select file under this path - selectSingleRecord(subRecord, true); - } else { - // select subdirectory - selectedDirectories.value.push(subRecord); - } + browseRemoteFiles(record.url, recursive) + .then((incoming) => { + incoming.entries.forEach((item) => { + // construct record + const subRecord = entryToRecord(item); + if (subRecord.isLeaf) { + // select file under this path + selectSingleRecord(subRecord, true); + } else { + // select subdirectory + selectedDirectories.value.push(subRecord); + } + }); + totalItems.value = incoming.totalMatches; + isBusy.value = false; + }) + .catch((error) => { + // undo the directory selection made above + unselectPath(record.url, false, record.id); + formatRows(); + errorMessage.value = errorMessageAsString(error); + isBusy.value = false; }); - totalItems.value = incoming.totalMatches; - isBusy.value = false; - }); } } From 38db02346f6ff95e116f0c3e27900860dd891fe5 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 15 Oct 2026 12:02:38 +0200 Subject: [PATCH 3/4] Keep email-derived OIDC username out of pipeline details Replace the username_from_email step with a wrapper around social_core's get_username. The previous step rewrote details["username"] for every pipeline step that followed, including for an existing account that associate_by_email_if_logged_in later matched by email. From 26.1 the sync_user_profile step applies details["username"] to that account with fixed_delegated_auth, so the first OIDC login would rename it. The wrapper passes the email-derived name only to social_core's get_username. Replace the mock-based tests, which relied on strategy.storage (social-core 6 resolves storage via get_storage), with callback tests through the real pipeline. --- lib/galaxy/authnz/psa_authnz.py | 33 ++++---- .../authnz/test_deferred_user_creation.py | 75 +++++++++++++++++++ test/unit/authnz/test_psa_authnz.py | 36 --------- 3 files changed, 94 insertions(+), 50 deletions(-) diff --git a/lib/galaxy/authnz/psa_authnz.py b/lib/galaxy/authnz/psa_authnz.py index 3c0f2d925d9..d75baad5440 100644 --- a/lib/galaxy/authnz/psa_authnz.py +++ b/lib/galaxy/authnz/psa_authnz.py @@ -20,6 +20,7 @@ from social_core.actions import ( do_disconnect, ) from social_core.backends.utils import get_backend +from social_core.pipeline.user import get_username as social_core_get_username from social_core.strategy import BaseStrategy from social_core.utils import ( module_member, @@ -119,12 +120,11 @@ AUTH_PIPELINE = ( "galaxy.authnz.psa_authnz.verify", # Checks if the current social-account is already associated in the site. "social_core.pipeline.social_auth.social_user", - # Custom Galaxy step: Fall back to the email's local part when the IdP sends - # no username claim; otherwise get_username substitutes a random string. - "galaxy.authnz.psa_authnz.username_from_email", - # Make up a username for this person, appends a random string at the end if - # there's any collision. - "social_core.pipeline.user.get_username", + # Custom Galaxy step: Make up a username for this person, appends a random + # string at the end if there's any collision. Wraps social_core's + # get_username to fall back to the email's local part when the IdP sends no + # username claim, instead of a random string. + "galaxy.authnz.psa_authnz.get_username", # Send a validation email to the user to verify its email address. # 'social_core.pipeline.mail.mail_validation', # Custom Galaxy step: Associates by email only if user is logged in, @@ -710,16 +710,21 @@ def contains_required_data(response=None, is_new=False, backend=None, **kwargs): raise MalformedContents(err_msg=f"Missing refresh token. {hint_msg}") -def username_from_email(details=None, user=None, **kwargs): +def get_username(strategy, details, backend, user=None, *args, **kwargs): """ - Pipeline step: derive a username from the email's local part for new users - whose IdP sends no username claim (e.g. no ``preferred_username``). + Pipeline step: social_core's get_username, deriving the username from the + email's local part for new users whose IdP sends no username claim (e.g. no + ``preferred_username``). + + The derived name is only passed to social_core's get_username; ``details`` + is left unchanged for later steps, so it is never applied to an existing + user found by email. """ - if user or not details or details.get("username") or not details.get("email"): - return - username = ready_name_for_url(details["email"].split("@")[0]).lower() - if username: - return {"details": {**details, "username": username}} + if not user and not details.get("username") and details.get("email"): + username = ready_name_for_url(details["email"].split("@")[0]).lower() + if username: + details = {**details, "username": username} + return social_core_get_username(strategy, details, backend, user, *args, **kwargs) def verify(strategy=None, response=None, details=None, **kwargs): diff --git a/test/unit/authnz/test_deferred_user_creation.py b/test/unit/authnz/test_deferred_user_creation.py index bb5da762618..6eaddacb5fc 100644 --- a/test/unit/authnz/test_deferred_user_creation.py +++ b/test/unit/authnz/test_deferred_user_creation.py @@ -19,6 +19,7 @@ from webob import Request from galaxy import model from galaxy.app_unittest_utils.galaxy_mock import MockTrans +from galaxy.authnz import psa_authnz from galaxy.authnz.flow_state import ( FlowState, InvalidFlow, @@ -362,3 +363,77 @@ def test_association_failure_cannot_reuse_confirmation(trans): assert counts(trans) == (1, 1, 0) with pytest.raises(InvalidFlow): adapter(trans).create_user(identifier, trans, "/") + + +def without_username_claim(trans, provider, email): + trans.app.authnz_manager.oidc_backends_config["keycloak"]["require_create_confirmation"] = False + trans.app.config.enable_account_interface = True + for source in (provider.claims, provider.userinfo): + source.update({"email": email, "preferred_username": None}) + + +def oidc_user(trans, email): + return trans.sa_session.query(model.User).filter_by(email=email).one() + + +def test_new_user_without_username_claim_uses_email_local_part(trans, provider): + without_username_claim(trans, provider, "Jane.Doe+lab@example.com") + callback(trans, start(trans, provider)) + assert oidc_user(trans, "jane.doe+lab@example.com").username == "janedoelab" + + +def test_email_local_part_username_collision_gets_suffix(trans, provider): + trans.app.user_manager.create(email="other@example.com", username="janedoe") + without_username_claim(trans, provider, "jane.doe@example.com") + callback(trans, start(trans, provider)) + username = oidc_user(trans, "jane.doe@example.com").username + assert username.startswith("janedoe") + assert username != "janedoe" + + +def test_username_claim_takes_precedence_over_email(trans, provider): + without_username_claim(trans, provider, "jane.doe@example.com") + provider.userinfo["preferred_username"] = "jdoe" + callback(trans, start(trans, provider)) + assert oidc_user(trans, "jane.doe@example.com").username == "jdoe" + + +def test_unusable_email_local_part_keeps_random_username(trans, provider): + without_username_claim(trans, provider, "äö@example.com") + callback(trans, start(trans, provider)) + username = oidc_user(trans, "äö@example.com").username + assert len(username) == 32 + int(username, 16) + + +def test_confirmation_without_username_claim_uses_email_local_part(trans, provider): + without_username_claim(trans, provider, "jane.doe@example.com") + trans.app.authnz_manager.oidc_backends_config["keycloak"]["require_create_confirmation"] = True + redirect, user = callback(trans, start(trans, provider)) + assert user is None + identifier = parse_qs(urlsplit(redirect).query)["confirmation_id"][0] + _, user = adapter(trans).create_user(identifier, trans, "/") + assert user.username == "janedoe" + + +def test_existing_user_found_by_email_keeps_username_details(trans, provider, monkeypatch): + # Later pipeline steps (e.g. profile sync) must not see the email-derived + # username as an IdP claim and apply it to the existing account. + trans.app.user_manager.create(email="jane.doe@example.com", username="custom-name") + without_username_claim(trans, provider, "jane.doe@example.com") + trans.app.config.fixed_delegated_auth = True + trans.app.config.enable_account_interface = False + monkeypatch.setattr(trans.app, "notification_manager", Mock(notifications_enabled=False), raising=False) + seen_details = [] + decode_access_token = psa_authnz.decode_access_token + + def record_details(**kwargs): + seen_details.append(kwargs["details"]) + return decode_access_token(**kwargs) + + monkeypatch.setattr(psa_authnz, "decode_access_token", record_details) + callback(trans, start(trans, provider)) + user = oidc_user(trans, "jane.doe@example.com") + assert user.username == "custom-name" + assert trans.sa_session.query(model.UserAuthnzToken).one().user_id == user.id + assert [details["username"] for details in seen_details] == [None] diff --git a/test/unit/authnz/test_psa_authnz.py b/test/unit/authnz/test_psa_authnz.py index 72676c8f626..c6263274886 100644 --- a/test/unit/authnz/test_psa_authnz.py +++ b/test/unit/authnz/test_psa_authnz.py @@ -26,7 +26,6 @@ from jwt import ( InvalidSignatureError, ) from social_core.backends.open_id_connect import OpenIdConnectAuth -from social_core.pipeline.user import get_username from sqlalchemy import create_engine from sqlalchemy.orm import Session @@ -37,7 +36,6 @@ from galaxy.authnz.psa_authnz import ( AUTH_PIPELINE, decode_access_token, PSAAuthnz, - username_from_email, ) @@ -369,37 +367,3 @@ def test_oidc_config_custom_auth_pipeline_and_extra(mock_oidc_config_file, mock_ app_config=mock_app.config, ) assert psa_authnz.config["SOCIAL_AUTH_PIPELINE"] == custom_auth_pipeline + tuple(custom_auth_pipeline_extra) - - -def _run_get_username(details): - backend = MagicMock() - backend.setting.side_effect = lambda name, default=None: default - strategy = MagicMock() - strategy.storage.user.username_max_length.return_value = 255 - strategy.storage.user.clean_username.side_effect = lambda value: value - strategy.storage.user.user_exists.return_value = False - return get_username(strategy, details, backend)["username"] - - -def test_username_from_email_without_username_claim(): - details = {"email": "Jane.Doe+lab@example.org", "username": None} - result = username_from_email(details=details) - assert result["details"]["username"] == "janedoelab" - assert _run_get_username(result["details"]) == "janedoelab" - - -def test_username_from_email_keeps_username_claim(): - details = {"email": "jane@example.org", "username": "jdoe"} - assert username_from_email(details=details) is None - assert _run_get_username(details) == "jdoe" - - -def test_username_from_email_skips_existing_user(): - details = {"email": "jane@example.org", "username": None} - assert username_from_email(details=details, user=MagicMock()) is None - - -def test_username_from_email_runs_before_get_username(): - assert AUTH_PIPELINE.index("galaxy.authnz.psa_authnz.username_from_email") < AUTH_PIPELINE.index( - "social_core.pipeline.user.get_username" - ) From 98e7a7215f28cb73b78612f9f78c9ffd8f9fedc8 Mon Sep 17 00:00:00 2001 From: Arash <2973722+arash77@users.noreply.github.com> Date: Thu, 15 Oct 2026 12:57:59 +0200 Subject: [PATCH 4/4] Show the folder selection error as a toast and keep the folder listed Replacing the listing with the error made Back show the file sources while the dialog was still deeper in the tree. --- client/src/components/FilesDialog/FilesDialog.test.ts | 8 +++++++- client/src/components/FilesDialog/FilesDialog.vue | 7 +++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/client/src/components/FilesDialog/FilesDialog.test.ts b/client/src/components/FilesDialog/FilesDialog.test.ts index 5fcfc628f02..ef91690b8f4 100644 --- a/client/src/components/FilesDialog/FilesDialog.test.ts +++ b/client/src/components/FilesDialog/FilesDialog.test.ts @@ -49,6 +49,11 @@ import SelectionDialog from "@/components/SelectionDialog/SelectionDialog.vue"; vi.mock("app"); +const { toastError } = vi.hoisted(() => ({ toastError: vi.fn() })); +vi.mock("@/composables/toast", () => ({ + useToast: () => ({ error: toastError }), +})); + vi.mock("@/composables/config", () => ({ useConfig: vi.fn(() => ({ config: { ftp_upload_site: "Test ftp upload site" }, @@ -273,7 +278,8 @@ describe("FilesDialog, file mode", () => { suppressDebugConsole(); // expecting error message. await utils.clickOn(utils.findRenderedDirectory(directoryId)); - utils.expectErrorMessage(); + expect(toastError).toHaveBeenCalledWith(someErrorText); + expect(utils.getRenderedRows().length).toBe(pdbResponse.length); expect(utils.getSelectionDialog().props("isBusy")).toBe(false); utils.expectOkButtonDisabled(); }); diff --git a/client/src/components/FilesDialog/FilesDialog.vue b/client/src/components/FilesDialog/FilesDialog.vue index c3d7d06d035..0b975752633 100644 --- a/client/src/components/FilesDialog/FilesDialog.vue +++ b/client/src/components/FilesDialog/FilesDialog.vue @@ -21,6 +21,7 @@ import { } from "@/components/SelectionDialog/selectionTypes"; import { useConfig } from "@/composables/config"; import { useFileSources } from "@/composables/fileSources"; +import { useToast } from "@/composables/toast"; import { useUrlTracker } from "@/composables/urlTracker"; import { useFileSourceTemplatesStore } from "@/stores/fileSourceTemplatesStore"; import { errorMessageAsString } from "@/utils/simple-error"; @@ -32,6 +33,7 @@ import GButton from "../BaseComponents/GButton.vue"; import SelectionDialog from "@/components/SelectionDialog/SelectionDialog.vue"; const filesSources = useFileSources(); +const toast = useToast(); interface FilesDialogProps { /** Callback function to be called passing the results when selection is complete */ @@ -199,10 +201,11 @@ function selectDirectoryRecursive(record: SelectionItem) { isBusy.value = false; }) .catch((error) => { - // undo the directory selection made above + // undo the directory selection made above; formatRows also disables OK again unselectPath(record.url, false, record.id); formatRows(); - errorMessage.value = errorMessageAsString(error); + // a toast keeps the folder listed, so files can still be picked one by one + toast.error(errorMessageAsString(error)); isBusy.value = false; }); }