Merge pull request #24162 from jdavcs/merge_26.1_into_26.2

[26.2] Merge 26.1 into 26.2

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", () => {

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;
-        });
     }
 }
 

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")

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]