Environmentvariableextension securitymanager tests #242

Hancho2009 · 2020-04-25T14:23:10Z

resolves #241

I hereby agree to the terms of the JUnit Pioneer Contributor License Agreement.

Bukama · 2020-04-25T17:13:46Z

For documentation:

Task :test FAILED
2020-04-25 14:51:44,487 pool-1-thread-1 ERROR Unable to unregister MBeans java.security.AccessControlException: access denied ("javax.management.MBeanServerPermission" "createMBeanServer")

sonarcloud · 2020-04-25T21:03:38Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

Hancho2009 · 2020-04-25T21:06:55Z

src/test/java/org/junitpioneer/jupiter/EnvironmentVariableUtilsTests.java

+	 * because junit uses reflection and the default SecurityManager prevents that.
+	 */
+	private void executeWithSecurityManager(Runnable runnable) {
+		Policy.getPolicy().refresh();


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

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?

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.

Hancho2009 · 2020-04-25T21:20:46Z

I am very inexperienced with the SecurityManager. If someone has better ideas how to handle this please share it with me.
I do not really like the *.policy files and how I can only give a certain code base permission but what I really want is to remove a certain (or all) permission(s) from the code base I am testing and not effect the others like gradles or junits.
I thought about implementing a SecurityManager for that but it seems a little to much for this small case. it would be cool if there where an junit extension to control policy permissions without files in the contex of a test i.e. like the @SetSystemProperty annotation.

aepfli · 2020-04-28T09:29:53Z

sidenote: i think this security thingy quiet interesting, and i am curious, beside this usecase with the policy - do you/we think that this might be a possible junit extension? we could generate a ticket out of that, and evaluate options, but it looks like something which could be useful. just an idea i came up while reading through this

Hancho2009 · 2020-04-28T13:57:49Z

sidenote: i think this security thingy quiet interesting, and i am curious, beside this usecase with the policy - do you/we think that this might be a possible junit extension? we could generate a ticket out of that, and evaluate options, but it looks like something which could be useful. just an idea i came up while reading through this

In general I could imagine such an extension but as I stated above I do not have much experience with the SecurityManager so I hope someone with more of a grasp on the topic can give some input. I think it would be good to create a ticket/issues for that to have a place for discussions if this PR will go away.

Bukama

To be honest I don't know anything about this topic and I don't know if I find time to read into it. So I ask @aepfli and @nicolaiparlog for their review.

aepfli · 2020-05-12T16:33:47Z

To be honest I don't know anything about this topic and I don't know if I find time to read into it. So I ask @aepfli and @nicolaiparlog for their review.

haha me neither - i will try to check today

Michael1993 · 2020-07-11T12:21:31Z

src/test/java/org/junitpioneer/jupiter/EnvironmentVariableUtilsTests.java

+	 */
+	private void executeWithSecurityManager(Runnable runnable) {
+		Policy.getPolicy().refresh();
+		SecurityManager securityManager = System.getSecurityManager();


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'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.

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

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

Seems clear to me?

Update

…extension-securitymanager-tests # Conflicts: # src/test/java/org/junitpioneer/jupiter/EnvironmentVariableUtilsTests.java

- changed exception handling so no exception is hidden if could not modify environment variables - adjust comment setInSystemEnvClass because it also works on osx

sonarcloud · 2020-07-14T20:10:58Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

nipafx · 2020-07-14T20:12:24Z

Finally took some time to look at this, @Hancho2009. Sorry for letting it sit for nigh three months. Looks good to me! 😃

Proposed commit message:

Add tests with enabled SecurityManager to env extension (#241 / #242)

The documentation mentions that without proper permissions an enabled
security manager will not give access to the internals we need to
change environment variables. These tests confirm that.

Closes: #241
PR: #242

Bukama

LGTM

Hancho2009 · 2020-07-15T06:54:51Z

@nicolaiparlog no problem

Hancho2009 added 3 commits April 25, 2020 15:59

Add tests for EnvironmentVariableUtilsTests with SecurityManager

3861384

adjust comment in policy files

e5db872

add permission for gradle

61099a7

Hancho2009 added 3 commits April 25, 2020 22:49

fix gradle problesm and some refactroing

fd69896

fix java 14 problem with assertj logger permission

2334364

fix not enough permissions problem during test execution

acca96a

Hancho2009 commented Apr 25, 2020

View reviewed changes

Bukama reviewed May 9, 2020

View reviewed changes

Bukama requested a review from aepfli June 6, 2020 08:25

Michael1993 reviewed Jul 11, 2020

View reviewed changes

Hancho2009 and others added 8 commits July 11, 2020 21:31

renamed local variable CR

6c46b87

Merge pull request #2 from junit-pioneer/master

d668163

Update

Merge remote-tracking branch 'origin/master' into environmentvariable…

41edd0d

…extension-securitymanager-tests # Conflicts: # src/test/java/org/junitpioneer/jupiter/EnvironmentVariableUtilsTests.java

- class should not be public api

e726aca

- changed exception handling so no exception is hidden if could not modify environment variables - adjust comment setInSystemEnvClass because it also works on osx

format block comment

50ea8ee

format block comment

c391d1f

Reorder code and add comment

fdd498d

Add contribution to README

c4599f4

Bukama approved these changes Jul 14, 2020

View reviewed changes

nipafx merged commit bc4d50b into junit-pioneer:master Jul 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Environmentvariableextension securitymanager tests #242

Environmentvariableextension securitymanager tests #242

Hancho2009 commented Apr 25, 2020

Bukama commented Apr 25, 2020

sonarcloud bot commented Apr 25, 2020

Hancho2009 Apr 25, 2020

Michael1993 Jul 11, 2020 •

edited

Michael1993 Jul 11, 2020 •

edited

Hancho2009 commented Apr 25, 2020 •

edited

aepfli commented Apr 28, 2020

Hancho2009 commented Apr 28, 2020

Bukama left a comment

aepfli commented May 12, 2020

Michael1993 Jul 11, 2020

Bukama Jul 11, 2020

Michael1993 Jul 11, 2020

sonarcloud bot commented Jul 14, 2020

nipafx commented Jul 14, 2020

Bukama left a comment

Hancho2009 commented Jul 15, 2020

Environmentvariableextension securitymanager tests #242

Environmentvariableextension securitymanager tests #242

Conversation

Hancho2009 commented Apr 25, 2020

resolves #241

Bukama commented Apr 25, 2020

sonarcloud bot commented Apr 25, 2020

Hancho2009 Apr 25, 2020

Choose a reason for hiding this comment

Michael1993 Jul 11, 2020 • edited

Choose a reason for hiding this comment

Michael1993 Jul 11, 2020 • edited

Choose a reason for hiding this comment

Hancho2009 commented Apr 25, 2020 • edited

aepfli commented Apr 28, 2020

Hancho2009 commented Apr 28, 2020

Bukama left a comment

Choose a reason for hiding this comment

aepfli commented May 12, 2020

Michael1993 Jul 11, 2020

Choose a reason for hiding this comment

Bukama Jul 11, 2020

Choose a reason for hiding this comment

Michael1993 Jul 11, 2020

Choose a reason for hiding this comment

sonarcloud bot commented Jul 14, 2020

nipafx commented Jul 14, 2020

Bukama left a comment

Choose a reason for hiding this comment

Hancho2009 commented Jul 15, 2020

Michael1993 Jul 11, 2020 •

edited

Michael1993 Jul 11, 2020 •

edited

Hancho2009 commented Apr 25, 2020 •

edited