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.

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

Environmentvariableextension securitymanager tests #242

Merged
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Copyright 2015-2020 the original author or authors.
*
* All rights reserved. This program and the accompanying materials are
* made available under the terms of the Eclipse Public License v2.0 which
* accompanies this distribution and is available at
*
* http://www.eclipse.org/legal/epl-v20.html
*/

package org.junitpioneer.jupiter;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.security.AccessControlException;
import java.security.Policy;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

@DisplayName("EnvironmentVariableUtils")
@ClearEnvironmentVariable(key = "TEST")
class EnvironmentVariableUtilsTests {

@Test
@SetSystemProperty(key = "java.security.policy", value = "file:src/test/resources/default-testing.policy")
void shouldThrowAccessControlExceptionWithDefaultSecurityManager() {
executeWithSecurityManager(() -> assertThatThrownBy(() -> EnvironmentVariableUtils.set("TEST", "TEST"))
.isInstanceOf(AccessControlException.class));
}

@Test
@SetSystemProperty(key = "java.security.policy", value = "file:src/test/resources/reflect-permission-testing.policy")
void shouldModifyEnvironmentVariableIfPermissionIsGiven() {
executeWithSecurityManager(() -> {
assertThatCode(() -> EnvironmentVariableUtils.set("TEST", "TEST")).doesNotThrowAnyException();
assertThat(System.getenv("TEST")).isEqualTo("TEST");
});
}

/*
* This needs to be done during the execution of the test method and cannot be moved to setup/tear down
* because junit uses reflection and the default SecurityManager prevents that.
*/
private void executeWithSecurityManager(Runnable runnable) {
Policy.getPolicy().refresh();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does something like that needs to be called after the SystemProperty java.security.policy is reverted after the test?

Copy link
Member

Choose a reason for hiding this comment

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

According to the official Oracle Java SE documentation

The refresh method causes the policy object to refresh/reload its data. This operation is implementation-dependent. For example, if the policy object stores its data in configuration files, calling refresh will cause it to re-read the configuration policy files. If a refresh operation is not supported, this method does nothing. Note that refreshed policy may not have an effect on classes in a particular ProtectionDomain. This is dependent on the Policy provider's implementation of the implies method and its PermissionCollection caching strategy.

To me, this reads like you should call it to properly reset the policy?

Copy link
Member

Choose a reason for hiding this comment

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

If you want to test if resetting the policy works as expected, you should probably add a new test for that and add @Order to your suite. Here is a blog post from Baeldung on that topic.

SecurityManager securityManager = System.getSecurityManager();
Copy link
Member

Choose a reason for hiding this comment

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

I'd have named this variable original. Everywhere it is used it is clear that it's a SecurityManager, so naming it securityManager is redundant - however System.setSecurityManager(original) clearly communicates that we reset (and not modify) the SecurityManager.
This is just a sidenote, don't feel the need to modify the PR just for this.

Copy link
Member

Choose a reason for hiding this comment

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

I'd have named this variable original. Everywhere it is used it is clear that it's a SecurityManager, so naming it securityManager is redundant - however System.setSecurityManager(original) clearly communicates that we reset (and not modify) the SecurityManager.
This is just a sidenote, don't feel the need to modify the PR just for this.

I would not rename the variable! It only seems redundant because of Java 8. With 10+, using var at this section it would not be clear what a variable with the name original holds.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, even with var it would look like this:

var original = System.getSecurityManager();
// ... other code ...
System.setSecurityManager(original);

Seems clear to me?

System.setSecurityManager(new SecurityManager());
try {
runnable.run();
}
finally {
System.setSecurityManager(securityManager);
}
}

}
7 changes: 7 additions & 0 deletions src/test/resources/default-testing.policy
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
grant {
// allow to reset the SecurityManager after the unit test
permission java.lang.RuntimePermission "setSecurityManager";

// necessary for gradle
permission javax.management.MBeanServerPermission "createMBeanServer";
};
17 changes: 17 additions & 0 deletions src/test/resources/reflect-permission-testing.policy
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
grant {
// allow to reset the SecurityManager after the unit test
permission java.lang.RuntimePermission "setSecurityManager";

// necessary for gradle
permission javax.management.MBeanServerPermission "createMBeanServer";

// needed for EnvironmentVariableUtilsTests#shouldModifyEnvironmentVariableIfPermissionIsGiven
permission java.lang.RuntimePermission "getenv.*";

// needed for assertj
permission java.util.logging.LoggingPermission "control";

// needed for EnvironmentVariableUtils
permission java.lang.RuntimePermission "accessDeclaredMembers";
permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
};