Issue 51524: Allowlist for acceptable types of file upload - automation test by labkey-sweta · Pull Request #2428 · LabKey/testAutomation

labkey-sweta · 2025-04-28T17:32:08Z

Rationale

Add automated tests for the Allowed Files list.

These tests will cover:

Validating the admin page for the Allowed Files List (http://localhost:8080/admin-allowList.view?type=FileExtension).
Test using the Files web part.
Test using Sample Types in LKS.
Test using Lists (tests attachments).
Test using Messages.

Has coverage for:
Issue 53039: Updating an allowed file extension should set a dirty bit on the page.
Issue 53027: Cannot insert a row into a sample type if Allowed File Extensions are set and no file is provided.

TeamCity runs are here.

Related Pull Requests

Changes

Add a AllowedFileExtensionAdminPage.
Update ShowAdminPage, BaseUpdatePage, FileBrowserHelper
Add AllowedFileExtensionAdminTest

…d tests

Add test to check for adding invalid extensions.

Update BaseUpdatePage to have a getter for the title and body.

…ing to check if fields remain populated).

…est and into BaseWebDriverTest. Add AllowedFileExtensionsTest in lims/biologics, add testAllowedFilesInSampleTypes. Add TestScrubber.clearListOfAllowedFileExtensions.

Add AllowedFileExtensionsTest.testAllowedFilesInWorkflow

labkey-jeckels · 2025-05-19T18:43:19Z

        waitForPipelineJobsToComplete(pipelineJobs, "Study Reload", expectError);
    }

+    protected void deleteAllAllowedFileExtension() throws IOException, CommandException


Suggested change

protected void deleteAllAllowedFileExtension() throws IOException, CommandException

/** Clears any file extensions that are set as the only allowable names, letting users upload any filename they like */

protected void deleteAllAllowedFileExtension() throws IOException, CommandException

labkey-jeckels · 2025-05-19T18:44:06Z

        }
    }
+
+    private void clearListOfAllowedFileExtensions() throws IOException, CommandException


Can we consolidate this with the copy in BaseWebDriverTest?

I've removed this method. The only place this code should now appear is in AllowedFileExtensionAdminPage.deleteAllAllowedFileExtension.

labkey-jeckels · 2025-05-19T18:45:35Z

+        // Not sure why we record two exceptions.
+        checkExpectedErrors(2);


Should no longer be needed

labkey-jeckels · 2025-05-19T18:45:48Z

+        // Not sure why we record two exceptions.
+        checkExpectedErrors(2);


Should no longer be needed

labkey-tchad

The instructions on the admin page say that ".gz" won't allow ".tar.gz" files; should we verify that is true?

labkey-tchad · 2025-05-21T17:33:44Z

+    protected void deleteAllAllowedFileExtension() throws IOException, CommandException
+    {
+        SimplePostCommand command = new SimplePostCommand("admin", "deleteAllValues");
+        Map<String, Object> params = new HashMap<>();
+        params.put("type", "FileExtension");
+        command.setParameters(params);
+        command.execute(createDefaultConnection(), "/");
+    }


This method should go in LabKeySiteWrapper. That would allow TestScrubber to access it.
Alternatively, you could make it a static method in AllowedFileExtensionAdminPage (with a Connection parameter).

I've moved it AllowedFileExtensionsAdminPage and made it static. I think it might be more discoverable there.

labkey-tchad · 2025-05-21T17:41:28Z

+
+    public static AllowedFileExtensionAdminPage beginAt(WebDriverWrapper webDriverWrapper)
+    {
+        webDriverWrapper.beginAt(WebTestHelper.buildURL("admin", "allowList"));


The allowList page has several flavors, need to specify the FileExtension version.

Suggested change

webDriverWrapper.beginAt(WebTestHelper.buildURL("admin", "allowList"));

webDriverWrapper.beginAt(WebTestHelper.buildURL("admin", "allowList", Map.of("type", "FileExtension")));

labkey-tchad · 2025-05-21T18:08:06Z

+    public String clickSaveExpectingError()
+    {
+        String errorText;
+        elementCache().saveExtension.click();
+        errorText = waitForElement(Locators.labkeyError).getText();
+        clearCache();
+        return errorText;
+    }
+
+    public AllowedFileExtensionAdminPage clickSaveExtension()
+    {
+        elementCache().saveExtension.click();
+        clearCache();
+        return this;
+    }


All of the buttons (delete, save, and update) trigger a page load, regardless of whether there is an error. These should all wait for a page load and error-free versions should verify that there is no error.

Suggested change

public String clickSaveExpectingError()

{

String errorText;

elementCache().saveExtension.click();

errorText = waitForElement(Locators.labkeyError).getText();

clearCache();

return errorText;

}

public AllowedFileExtensionAdminPage clickSaveExtension()

{

elementCache().saveExtension.click();

clearCache();

return this;

}

private AllowedFileExtensionAdminPage clickButtonNoError(WebElement button)

{

clickAndWait(button);

clearCache();

assertNoLabKeyErrors();

return this;

}

private String clickButtonExpectingError(WebElement button)

{

clickAndWait(button);

clearCache();

return waitForElement(Locators.labkeyError).getText();

}

public String clickSaveExpectingError()

{

return clickButtonExpectingError(elementCache().saveExtension);

}

public AllowedFileExtensionAdminPage clickSaveExtension()

{

return clickButtonNoError(elementCache().saveExtension);

}

And so on for update and delete.

labkey-tchad · 2025-05-21T18:19:31Z

+        elementCache().deleteAll.click();
+
+        if (acceptAlert)
+        {
+            acceptAlert();
+            shortWait().withMessage("'Delete All' button should have gone away.")
+                    .until(ExpectedConditions.stalenessOf(elementCache().deleteAll));
+        }
+        else
+        {
+            cancelAlert();
+        }
+
+        clearCache();
+        return this;


This staleness check will always pass because there is a page load. Need to clear the cache and check the button's visibility.

Suggested change

elementCache().deleteAll.click();

if (acceptAlert)

{

acceptAlert();

shortWait().withMessage("'Delete All' button should have gone away.")

.until(ExpectedConditions.stalenessOf(elementCache().deleteAll));

}

else

{

cancelAlert();

}

clearCache();

return this;

if (acceptAlert)

{

doAndWaitForPageLoad(() -> {

elementCache().deleteAll.click();

acceptAlert();

});

clearCache();

assertFalse("Delete All button should not be present after deleting all extensions", elementCache().deleteAll.isDisplayed());

}

else

{

elementCache().deleteAll.click();

cancelAlert();

assertTrue("Delete All button should be present", elementCache().deleteAll.isDisplayed());

}

return this;

labkey-tchad · 2025-05-21T18:28:07Z

+        new PortalHelper(getDriver()).addWebPart("Files");
+        new PortalHelper(getDriver()).addWebPart("Sample Types");
+        new PortalHelper(getDriver()).addWebPart("Lists");
+        new PortalHelper(getDriver()).addWebPart("Messages List");


Each addWebPart enters and exits page admin mode. Wrap in doInAdminMode to avoid all the extra navigations.

Suggested change

new PortalHelper(getDriver()).addWebPart("Files");

new PortalHelper(getDriver()).addWebPart("Sample Types");

new PortalHelper(getDriver()).addWebPart("Lists");

new PortalHelper(getDriver()).addWebPart("Messages List");

new PortalHelper(driver).doInAdminMode(ph -> {

ph.addWebPart("Files");

ph.addWebPart("Sample Types");

ph.addWebPart("Lists");

ph.addWebPart("Messages List");

});

labkey-tchad · 2025-05-21T18:42:29Z

+        allowedFileExtensionAdminPage = goToAdminConsole().clickAllowedFileExtensions();
+        allowedFileExtensionAdminPage.setExtension("not .an extension");
+        String expectedValue = "File extension must start with a '.'";
+        String actualValue = allowedFileExtensionAdminPage.clickSaveExpectingError();
+        checker().withScreenshot()
+                .verifyEquals("Incorrect error message for invalid extension.",
+                        expectedValue, actualValue);
+
+        allowedFileExtensionAdminPage.setExtension(allowedTypes.get(0));
+        expectedValue = String.format("'%s' already exists. Duplicate values not allowed.", allowedTypes.get(0));
+        actualValue = allowedFileExtensionAdminPage.clickSaveExpectingError();
+        checker().withScreenshot()
+                .verifyEquals("Incorrect error message for duplicate extension.",
+                        expectedValue, actualValue);
+
+        allowedFileExtensionAdminPage.setExtension(allowedTypes.get(1).toUpperCase());
+        expectedValue = String.format("'%s' already exists. Duplicate values not allowed.", allowedTypes.get(1).toUpperCase());
+        actualValue = allowedFileExtensionAdminPage.clickSaveExpectingError();
+        checker().withScreenshot()
+                .verifyEquals("Incorrect error message for duplicate extension but different case.",
+                        expectedValue, actualValue);
+
+        allowedFileExtensionAdminPage.setExtension("");
+        expectedValue = "File extension must not be blank.";
+        actualValue = allowedFileExtensionAdminPage.clickSaveExpectingError();
+        checker().withScreenshot()
+                .verifyEquals("Incorrect error message for blank extension value.",
+                        expectedValue, actualValue);


Consider moving the admin page validation to a separate test case (duplicate extensions, updating extensions, invalid extensions, delete all cancelling, etc.)

I have moved most of the tests cases that check the admin page functionality into a separate class:

Check that allowed file extensions are listed in alphabetical order.

Canceling out of 'Delete All'.

Validate extension value must start with a '.'

Validate duplicate extensions are not allowed.

Validate blank extension type is not allowed.

I did leave in test cases that validate actions on the admin page behave as expected in the project:

Remove an allowed file type and validate files of that type cannot be uploaded.

Click 'Delete All' and validate any file type can be uploaded.

Edit an allowed extension, .xls to .xlsx, and validate .xlsx files can be uploaded, but .xls cannot.

As a side note it is not possible to test for dirty bit in LKS with selenium. See issue 38785.

labkey-tchad · 2025-05-21T18:46:47Z

+        _listHelper.createList(getProjectName(), listName, keyField,
+                new FieldDefinition(attachmentField, FieldDefinition.ColumnType.Attachment));


Create list via API.

Suggested change

_listHelper.createList(getProjectName(), listName, keyField,

new FieldDefinition(attachmentField, FieldDefinition.ColumnType.Attachment));

new IntListDefinition(listName, keyField)

.addField(new FieldDefinition(attachmentField, FieldDefinition.ColumnType.Attachment))

.create(getProjectName(), createDefaultConnection());

labkey-tchad · 2025-05-21T18:51:10Z

+        FileInput el = FileInput.FileInput(Locator.name("quf_" + attachmentField), getDriver()).findWhenNeeded();
+        executeScript("arguments[0].value = '';", el.getComponentElement());


Does WebElement.clear work for this?

Suggested change

FileInput el = FileInput.FileInput(Locator.name("quf_" + attachmentField), getDriver()).findWhenNeeded();

executeScript("arguments[0].value = '';", el.getComponentElement());

FileInput el = FileInput.FileInput(Locator.name("quf_" + attachmentField), getDriver()).findWhenNeeded();

el.getComponentElement().clear();

If it does, consider adding a clear method to FileInput.

It appears to. I've added FileInput.clear.

labkey-jeckels · 2025-05-21T20:07:02Z

The instructions on the admin page say that ".gz" won't allow ".tar.gz" files; should we verify that is true?

This is a good idea - we should cover a "double extension" like this.

labkey-chrisj

I don't have anything pressing to add, looks good to me tho

Move deleteAllAllowedFileExtensions to the AllowedFileExtensionAdminPage Consolidate button code in AllowedFileExtensionAdminPage Remove calls to checkExpectedErrors. Adding FileInput.clear method. Many other code review feedback as well.

labkey-danield · 2025-05-31T01:16:14Z

The instructions on the admin page say that ".gz" won't allow ".tar.gz" files; should we verify that is true?

This is a good idea - we should cover a "double extension" like this.

I've added a tar.gz file to the mix.

… (AllowedFileExtensionsPageTest). Add a base class that has some shared code.

Cleaned up the biologics AllowedFileExtensionsTest (removed reset errors, used existing methods, updated expected error message). Added AppRegisterCellLinePage.setColumnValue that takes a file parameter. Added AppRegisterCellLinePage.clickSubmitExpectingError. Updated ThreadAttachment.clickRemove to return a confirmation dialog. Added ThreadEditor.clickCreateBtnExpectingError.

Fixed some typos in comments.

labkey-sweta and others added 9 commits April 24, 2025 11:57

Issue 51524: Allowlist for acceptable types of file upload - automate…

69c0be8

…d tests

Merge branch 'develop' into fb_allowFileExtensionAutoTest

88070ee

Reworking some of the allowed file extension tests.

e17dfa3

Merge branch 'develop' into fb_allowFileExtensionAutoTest

ec58d43

Merge branch 'develop' into fb_allowFileExtensionAutoTest

f5455a0

Merge branch 'develop' into fb_allowFileExtensionAutoTest

159dead

Merge branch 'develop' into fb_allowFileExtensionAutoTest

f6aaa24

Finish tests for SampleTypes and Lists.

fafd0f7

Add test to check for adding invalid extensions.

Add tests for Messages.

6951263

Update BaseUpdatePage to have a getter for the title and body.

labkey-danield marked this pull request as ready for review May 8, 2025 01:45

labkey-danield self-assigned this May 8, 2025

labkey-danield added 4 commits May 8, 2025 10:38

Merge branch 'develop' into fb_allowFileExtensionAutoTest

1a454d4

Change test workflow to accommodate resolution of issue 53026 (not go…

e8514f5

…ing to check if fields remain populated).

Merge branch 'develop' into fb_allowFileExtensionAutoTest

795eb4e

Move deleteAllAllowedFileExtensions out of AllowedFileExtensionAdminT…

204f26f

…est and into BaseWebDriverTest. Add AllowedFileExtensionsTest in lims/biologics, add testAllowedFilesInSampleTypes. Add TestScrubber.clearListOfAllowedFileExtensions.

labkey-jeckels mentioned this pull request May 16, 2025

Issue 53027: Cannot insert a row into a sample type if Allowed File Extensions set, no file is provided LabKey/platform#6677

Merged

Add check for dirty bit behavior to AllowedFileExtensionAdminTest

c920717

Add AllowedFileExtensionsTest.testAllowedFilesInWorkflow

labkey-danield requested review from labkey-chrisj, labkey-jeckels and labkey-tchad May 16, 2025 21:58

Merge branch 'develop' into fb_allowFileExtensionAutoTest

544e48b

labkey-jeckels reviewed May 19, 2025

View reviewed changes

labkey-tchad requested changes May 21, 2025

View reviewed changes

Merge branch 'develop' into fb_allowFileExtensionAutoTest

9b8040a

labkey-tchad self-requested a review May 22, 2025 22:26

labkey-tchad approved these changes May 22, 2025

View reviewed changes

labkey-chrisj reviewed May 28, 2025

View reviewed changes

labkey-danield added 2 commits May 29, 2025 16:57

Merge branch 'develop' into fb_allowFileExtensionAutoTest

b820591

Add a tar.gz file to the mix.

5ec98ae

Move deleteAllAllowedFileExtensions to the AllowedFileExtensionAdminPage Consolidate button code in AllowedFileExtensionAdminPage Remove calls to checkExpectedErrors. Adding FileInput.clear method. Many other code review feedback as well.

Add a tar.gz file to the mix.

e8eb091

Move deleteAllAllowedFileExtensions to the AllowedFileExtensionAdminPage Consolidate button code in AllowedFileExtensionAdminPage Remove calls to checkExpectedErrors. Adding FileInput.clear method. Many other code review feedback as well.

labkey-danield added 8 commits June 4, 2025 16:19

Merge branch 'develop' into fb_allowFileExtensionAutoTest

046b264

Move most of the Admin Page specific test cases into a separate class…

9bf8303

… (AllowedFileExtensionsPageTest). Add a base class that has some shared code.

Change test names.

d3dd6e6

Merge branch 'develop' into fb_allowFileExtensionAutoTest

421dec5

Merge branch 'develop' into fb_allowFileExtensionAutoTest

755177d

Merge branch 'develop' into fb_allowFileExtensionAutoTest

a209aa0

Update tests to validate the complete error message.

067c2a3

Fixed some typos in comments.

labkey-danield merged commit 504f0ef into develop Jun 10, 2025
6 checks passed

labkey-danield deleted the fb_allowFileExtensionAutoTest branch June 10, 2025 17:32

labkey-tchad mentioned this pull request Jun 11, 2025

Fix expected icon counts in FileBrowserIconTest #2485

Merged

labkey-tchad mentioned this pull request Jul 3, 2025

Use OS-specific file separator for expected file paths #2536

Merged

	protected void deleteAllAllowedFileExtension() throws IOException, CommandException
	/** Clears any file extensions that are set as the only allowable names, letting users upload any filename they like */
	protected void deleteAllAllowedFileExtension() throws IOException, CommandException

		// Not sure why we record two exceptions.
		checkExpectedErrors(2);

	webDriverWrapper.beginAt(WebTestHelper.buildURL("admin", "allowList"));
	webDriverWrapper.beginAt(WebTestHelper.buildURL("admin", "allowList", Map.of("type", "FileExtension")));

-        new PortalHelper(getDriver()).addWebPart("Files");
-        new PortalHelper(getDriver()).addWebPart("Sample Types");
-        new PortalHelper(getDriver()).addWebPart("Lists");
-        new PortalHelper(getDriver()).addWebPart("Messages List");
+        new PortalHelper(driver).doInAdminMode(ph -> {
+            ph.addWebPart("Files");
+            ph.addWebPart("Sample Types");
+            ph.addWebPart("Lists");
+            ph.addWebPart("Messages List");
+        });

		_listHelper.createList(getProjectName(), listName, keyField,
		new FieldDefinition(attachmentField, FieldDefinition.ColumnType.Attachment));

-        _listHelper.createList(getProjectName(), listName, keyField,
-                new FieldDefinition(attachmentField, FieldDefinition.ColumnType.Attachment));
+        new IntListDefinition(listName, keyField)
+            .addField(new FieldDefinition(attachmentField, FieldDefinition.ColumnType.Attachment))
+            .create(getProjectName(), createDefaultConnection());

		FileInput el = FileInput.FileInput(Locator.name("quf_" + attachmentField), getDriver()).findWhenNeeded();
		executeScript("arguments[0].value = '';", el.getComponentElement());

Conversation

labkey-sweta commented Apr 28, 2025 • edited by labkey-danield Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Related Pull Requests

Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labkey-tchad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labkey-danield Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labkey-jeckels commented May 21, 2025

Uh oh!

labkey-chrisj left a comment

Choose a reason for hiding this comment

Uh oh!

labkey-danield commented May 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

labkey-sweta commented Apr 28, 2025 •

edited by labkey-danield

Loading

labkey-danield Jun 5, 2025 •

edited

Loading