diff --git a/client/src/components/FilesDialog/FilesDialog.test.ts b/client/src/components/FilesDialog/FilesDialog.test.ts index d63158a5f43..05b9e5dcb72 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" }, @@ -267,6 +272,23 @@ 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)); + expect(toastError).toHaveBeenCalledWith(someErrorText); + expect(utils.getRenderedRows().length).toBe(pdbResponse.length); + 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 966ec0660f7..48894fbf26b 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"; @@ -33,6 +34,7 @@ import GAlert from "@/components/BaseComponents/GAlert.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 */ @@ -185,21 +187,30 @@ 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; formatRows also disables OK again + unselectPath(record.url, false, record.id); + formatRows(); + // a toast keeps the folder listed, so files can still be picked one by one + toast.error(errorMessageAsString(error)); + isBusy.value = false; }); - totalItems.value = incoming.totalMatches; - isBusy.value = false; - }); } } diff --git a/lib/galaxy/authnz/psa_authnz.py b/lib/galaxy/authnz/psa_authnz.py index ade22beec9b..484f414a615 100644 --- a/lib/galaxy/authnz/psa_authnz.py +++ b/lib/galaxy/authnz/psa_authnz.py @@ -21,7 +21,10 @@ from social_core.actions import ( do_disconnect, ) from social_core.backends.utils import get_backend -from social_core.pipeline.user import create_user as social_create_user +from social_core.pipeline.user import ( + create_user as social_create_user, + get_username as social_core_get_username, +) from social_core.storage import ( AssociationMixin, BaseStorage, @@ -159,9 +162,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", - # 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, @@ -832,6 +837,23 @@ def contains_required_data(response=None, is_new=False, backend=None, **kwargs): raise MalformedContents(err_msg=f"Missing refresh token. {hint_msg}") +def get_username(strategy, details, backend, user=None, *args, **kwargs): + """ + 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 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): 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_deferred_user_creation.py b/test/unit/authnz/test_deferred_user_creation.py index 69145fae215..a6101239ed8 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, @@ -375,3 +376,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]