[s7Ob7Zm7] Test that all load and import procedures respect security #360

vga91 · 2023-04-01T08:56:58Z

Added ImportAndLoadCoreSecurityTest.java with all test config, for both load.* and import.* procedures, since they share the same behaviors with the configs internal.dbms.cypher_ip_blocklist, apoc.import.file.enabled, apoc.import.file.use_neo4j_config and apoc.import.file.allow_read_from_filesystem
Deleted LoadCoreSecurityTest, as it has been replaced by ImportAndLoadSecurityTest
Added createTempFolder() to create a temp folder without having to use DatabaseManagementService and ClassRule TemporaryFolder
Changed SensitivePathGenerator.etcPasswd() as it didn't seem to work locally if the project located in a subfolder

gem-neo4j

This looks really good :) I know the card specified import and load, but how hard would it be to do this style for all our export as well? then we would be super tested :D

Just a few comments otherwise!

gem-neo4j · 2023-04-03T13:58:58Z

core/src/test/java/apoc/export/ImportAndLoadCoreSecurityTest.java

+    public static void setUp() throws IOException {
+        TestUtil.registerProcedure(db,
+                ImportJson.class, Xml.class, ImportCsv.class, ExportGraphML.class,
+                LoadJson.class, Xml.class);


Xml.class is here twice ^^

gem-neo4j · 2023-04-03T14:00:24Z

core/src/test/java/apoc/export/ImportAndLoadCoreSecurityTest.java

+
+    @Test
+    public void testIllegalFSAccessWithDifferentApocConfs() {
+        setFileApocConfigs(true, true, true);


Can a comment be added to explain which each boolean is setting so the test is easier to read as is?

Also would help explain why not all permutations are listed here, I can't directly see why we aren't also testing:

false, false, true
true, false, true
true, true, false

Actually I don't remember why I didn't test all permutations 😅... Anyway I've now added them all.

gem-neo4j · 2023-04-03T14:00:31Z

core/src/test/java/apoc/export/ImportAndLoadCoreSecurityTest.java

+
+    @Test
+    public void testImportFileDisabled() {
+        setFileApocConfigs(false, false, false);


Can a comment be added to explain which each boolean is setting so the test is easier to read as is?

gem-neo4j · 2023-04-03T14:06:21Z

core/src/test/java/apoc/export/ImportAndLoadCoreSecurityTest.java

+    @BeforeClass
+    public static void setUp() throws IOException {
+        TestUtil.registerProcedure(db,
+                ImportJson.class, Xml.class, ImportCsv.class, ExportGraphML.class,


Do we not want arrow procedures here as well?

Strange that the load graphml prcocedure is in the ExportGraphML class, maybe add a comment here as I was about to comment asking why an export procedure class was loaded into an import test 😅

Yes sorry, I thought I also added arrow, added.
And added a comment as well

gem-neo4j

Great, thanks for this :D Nice work!

…ecurity (#360)" This reverts commit ecec458.

…spect security (#360)" This reverts commit ecec458.

…spect security (#360)" (#376) This reverts commit ecec458.

…360) * [s7Ob7Zm7] Test that all load and import procedures respect security * [3QAbWQds] changed ExportCoreSecurityTest consistent with LoadImport one

…360) (#377) * [s7Ob7Zm7] Test that all load and import procedures respect security (#360) * [s7Ob7Zm7] Test that all load and import procedures respect security * [3QAbWQds] changed ExportCoreSecurityTest consistent with LoadImport one * [s7Ob7Zm7] changed tests * [s7Ob7Zm7] Update ImportAndLoadCoreSecurityTest.java * [s7Ob7Zm7] reduce logs * [s7Ob7Zm7] reduce logs in export tests

…eo4j/apoc#360) (neo4j/apoc#377) * [s7Ob7Zm7] Test that all load and import procedures respect security (#360) * [s7Ob7Zm7] Test that all load and import procedures respect security * [3QAbWQds] changed ExportCoreSecurityTest consistent with LoadImport one * [s7Ob7Zm7] changed tests * [s7Ob7Zm7] Update ImportAndLoadCoreSecurityTest.java * [s7Ob7Zm7] reduce logs * [s7Ob7Zm7] reduce logs in export tests

…eo4j/apoc#360) (neo4j/apoc#377) (#3617) * [s7Ob7Zm7] Test that all load and import procedures respect security (neo4j/apoc#360) (neo4j/apoc#377) * [s7Ob7Zm7] Test that all load and import procedures respect security (#360) * [s7Ob7Zm7] Test that all load and import procedures respect security * [3QAbWQds] changed ExportCoreSecurityTest consistent with LoadImport one * [s7Ob7Zm7] changed tests * [s7Ob7Zm7] Update ImportAndLoadCoreSecurityTest.java * [s7Ob7Zm7] reduce logs * [s7Ob7Zm7] reduce logs in export tests * [s7Ob7Zm7] 4.x changes

vga91 marked this pull request as draft April 1, 2023 08:57

vga91 force-pushed the test-loads-and-imports-configs branch from b2fcc27 to b8ca6f4 Compare April 3, 2023 07:05

vga91 marked this pull request as ready for review April 3, 2023 07:05

vga91 changed the title ~~Test that all load and import procedures respect security~~ [s7Ob7Zm7] Test that all load and import procedures respect security Apr 3, 2023

vga91 force-pushed the test-loads-and-imports-configs branch 2 times, most recently from 8f4b542 to f8bf9b0 Compare April 3, 2023 07:14

Lojjs assigned gem-neo4j Apr 3, 2023

gem-neo4j reviewed Apr 3, 2023

View reviewed changes

vga91 force-pushed the test-loads-and-imports-configs branch 2 times, most recently from afd8d11 to e6a545b Compare April 12, 2023 15:33

gem-neo4j approved these changes Apr 14, 2023

View reviewed changes

vga91 added 2 commits April 14, 2023 12:54

[s7Ob7Zm7] Test that all load and import procedures respect security

7097aa0

[3QAbWQds] changed ExportCoreSecurityTest consistent with LoadImport one

a074d54

vga91 force-pushed the test-loads-and-imports-configs branch from e6a545b to a074d54 Compare April 14, 2023 12:11

vga91 merged commit ecec458 into dev Apr 17, 2023

vga91 deleted the test-loads-and-imports-configs branch April 17, 2023 07:20

vga91 added a commit that referenced this pull request Apr 17, 2023

Revert "[s7Ob7Zm7] Test that all load and import procedures respect s…

6137a9f

…ecurity (#360)" This reverts commit ecec458.

vga91 mentioned this pull request Apr 17, 2023

[NOID] Revert "[s7Ob7Zm7] Test that all load and import procedures respect security" #376

Merged

vga91 added a commit that referenced this pull request Apr 17, 2023

[NOID] Revert "[s7Ob7Zm7] Test that all load and import procedures re…

2c9fb27

…spect security (#360)" This reverts commit ecec458.

vga91 added a commit that referenced this pull request Apr 17, 2023

[NOID] Revert "[s7Ob7Zm7] Test that all load and import procedures re…

ba98099

…spect security (#360)" (#376) This reverts commit ecec458.

vga91 mentioned this pull request May 31, 2023

[s7Ob7Zm7] Test that all load and import procedures respect security (#360) #377

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[s7Ob7Zm7] Test that all load and import procedures respect security #360

[s7Ob7Zm7] Test that all load and import procedures respect security #360

vga91 commented Apr 1, 2023 •

edited

Loading

gem-neo4j left a comment

gem-neo4j Apr 3, 2023

gem-neo4j Apr 3, 2023

gem-neo4j Apr 3, 2023

vga91 Apr 12, 2023

gem-neo4j Apr 3, 2023

vga91 Apr 12, 2023

gem-neo4j Apr 3, 2023

gem-neo4j Apr 3, 2023

vga91 Apr 12, 2023

gem-neo4j left a comment

[s7Ob7Zm7] Test that all load and import procedures respect security #360

[s7Ob7Zm7] Test that all load and import procedures respect security #360

Conversation

vga91 commented Apr 1, 2023 • edited Loading

gem-neo4j left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gem-neo4j left a comment

Choose a reason for hiding this comment

vga91 commented Apr 1, 2023 •

edited

Loading