Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WFCORE-6532] Add security manager to script tests #5706

Merged
merged 1 commit into from
Oct 16, 2023
Merged

[WFCORE-6532] Add security manager to script tests #5706

merged 1 commit into from
Oct 16, 2023

Conversation

xjusko
Copy link
Contributor

@xjusko xjusko commented Oct 10, 2023 •
edited by yersan
Loading

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Oct 10, 2023
jamezp
jamezp requested changes Oct 11, 2023
View reviewed changes
testsuite/scripts/src/test/java/org/wildfly/scripts/test/DomainScriptTestCase.java Outdated
@Override
void testScript(final ScriptProcess script) throws InterruptedException, TimeoutException, IOException {
script.start(HOST_CONTROLLER_CHECK, ServerHelper.DEFAULT_SERVER_JAVA_OPTS);
boolean isSecurityManagerEnabled = Boolean.parseBoolean(System.getProperty("security.manager"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should skip this check and just always test with the security manager enabled in one run. Currently we don't run security manager tests on Windows so we'd lose that testing.

testsuite/scripts/src/test/java/org/wildfly/scripts/test/DomainScriptTestCase.java Outdated
Comment on lines 43 to 46
final Collection<Object> result = new ArrayList<>(2);
result.add(Collections.emptyMap());
result.add(Collections.singletonMap("SECMGR", "true"));
return result;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could simply be return List.of(Map.of(), Map.of("SECMGR", "true"));.

testsuite/scripts/src/test/java/org/wildfly/scripts/test/StandaloneScriptTestCase.java Outdated
@@ -89,7 +91,13 @@ void testScript(final ScriptProcess script) throws InterruptedException, Timeout
// seem to work when a directory has a space. An error indicating the trailing quote cannot be found. Removing
// the `\ parts and just keeping quotes ends in the error shown in JDK-8215398.
Assume.assumeFalse(TestSuiteEnvironment.isWindows() && env.containsKey("GC_LOG") && script.getScript().toString().contains(" "));
script.start(STANDALONE_CHECK, env, ServerHelper.DEFAULT_SERVER_JAVA_OPTS);

boolean isSecurityManagerEnabled = Boolean.parseBoolean(System.getProperty("security.manager"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, we should always do a run with the security manager enabled.

@xjusko xjusko force-pushed the WFCORE-6532-add-security-manager-to-script-tests branch from d48fe23 to cc4aab7 Compare October 11, 2023 08:59
jamezp
jamezp requested changes Oct 11, 2023
View reviewed changes
testsuite/scripts/src/test/java/org/wildfly/scripts/test/DomainScriptTestCase.java Outdated
Comment on lines 47 to 48
String[] modifiedJavaOpts = Arrays.copyOf(ServerHelper.DEFAULT_SERVER_JAVA_OPTS, ServerHelper.DEFAULT_SERVER_JAVA_OPTS.length + 1);
modifiedJavaOpts[modifiedJavaOpts.length - 1] = "-secmgr";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was clear with my wording here. We shouldn't be adding the -secmgr parameter to the server arguments if we've already got it set in the environment variable. Essentially we could just do this:

script.start(HOST_CONTROLLER_CHECK, env, ServerHelper.DEFAULT_SERVER_JAVA_OPTS);

testsuite/scripts/src/test/java/org/wildfly/scripts/test/StandaloneScriptTestCase.java Outdated
Comment on lines 92 to 94
String[] modifiedJavaOpts = Arrays.copyOf(ServerHelper.DEFAULT_SERVER_JAVA_OPTS, ServerHelper.DEFAULT_SERVER_JAVA_OPTS.length + 1);
modifiedJavaOpts[modifiedJavaOpts.length - 1] = "-secmgr";
script.start(STANDALONE_CHECK, env, modifiedJavaOpts);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above here. The only change needed in this case is just to the data() method where we add the "SECMGR", "true" map entry.

@xjusko xjusko force-pushed the WFCORE-6532-add-security-manager-to-script-tests branch from cc4aab7 to 8a885e2 Compare October 12, 2023 05:50
@xjusko
Copy link
Contributor Author

xjusko commented Oct 12, 2023

I see, no problem, it should be fixed now.
Thanks for your help.

jamezp
jamezp requested changes Oct 12, 2023
View reviewed changes
testsuite/scripts/src/test/java/org/wildfly/scripts/test/DomainScriptTestCase.java Outdated
@@ -6,6 +6,10 @@
package org.wildfly.scripts.test;

import java.io.IOException;
import java.util.Arrays;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import is causing a checkstyle error as it's not used.

testsuite/scripts/src/test/java/org/wildfly/scripts/test/StandaloneScriptTestCase.java Outdated
@@ -8,9 +8,8 @@
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Arrays;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also causing a checkstyle issue and should be removed.

@xjusko xjusko force-pushed the WFCORE-6532-add-security-manager-to-script-tests branch from 8a885e2 to 527ab79 Compare October 12, 2023 16:58
@xjusko
Copy link
Contributor Author

xjusko commented Oct 12, 2023

Sorry, I don't know why it didn't show the checkstyle error when I compiled it.

@yersan yersan added the ready-for-merge This PR is ready to be merged and fulfills all requirements label Oct 16, 2023
@yersan yersan merged commit 19c4a26 into wildfly:main Oct 16, 2023
1 check passed
@yersan
Copy link
Collaborator

yersan commented Oct 16, 2023

Thanks @xjusko @jamezp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamezp jamezp jamezp approved these changes

Assignees
No one assigned
Labels
deps-ok Dependencies have been checked, and there are no significant changes ready-for-merge This PR is ready to be merged and fulfills all requirements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xjusko @yersan @jamezp