-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Enable security manager for active directory tests #112411
base: main
Are you sure you want to change the base?
Conversation
When active directory tests switched to using test rules for manager docker fixtures, the security manager was disabled. This commit re-enables the security manager, and isolates the docker setup/start to not run under security manager.
Pinging @elastic/es-security (Team:Security) |
Pinging @elastic/es-core-infra (Team:Core/Infra) |
* | ||
* @return A closeable object which restores the test security manager | ||
*/ | ||
@SuppressWarnings("removal") | ||
public static Closeable disableTestSecurityManager() { | ||
var caller = Thread.currentThread().getStackTrace()[2]; | ||
if (ESTestCase.class.getName().equals(caller.getClassName()) == false) { | ||
throw new SecurityException("Cannot disable test SecurityManager directly. Use @NoSecurityManager to disable on a test suite"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for core/infra: I've relaxed this requirement to enable broader use of disabling security manager for parts of testing. The most important thing is to ensure security manager is not disabled and forgotten. We already return a releasable object, and additional validation ensures we haven't forgotten to re-enable security manager between tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (after checking why tests fail)
* Temporarily dsiables security manager for a test. | ||
* | ||
* <p> This method is only callable by {@link org.elasticsearch.test.ESTestCase}. | ||
* Temporarily disables security manager for a test. | ||
* | ||
* @return A closeable object which restores the test security manager | ||
*/ | ||
@SuppressWarnings("removal") | ||
public static Closeable disableTestSecurityManager() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could change this to return Releaseable
so we don't need to catch IOException
in the methods below?
When active directory tests switched to using test rules for manager docker fixtures, the security manager was disabled. This commit re-enables the security manager, and isolates the docker setup/start to not run under security manager.