Fix genuine bugs found while cleaning up java/unused-parameter alerts

[vharseko]

Summary

Investigated all 49 open java/unused-parameter CodeQL alerts. 40 were dismissed on GitHub as false positive/won't-fix (interface/abstract method declarations, uniform dispatch signatures, TestNG DataProvider/Factory injection, Procrun stop(String[] args) convention, and public extensibility hooks). The remaining 9 pointed at real problems, fixed here:

• ADUserAccountControl: the private constructor assigned uac to both the uac and msDSUac fields instead of keeping them separate, so isAccountLockOut()/isPasswordExpired() never read the real value of AD's ms-DS-User-Account-Control-Computed attribute.
• JavaScriptExecutorFactory: accepted a ClassLoader but never applied it (unlike its Groovy sibling, which passes it into GroovyShell). JS scripts always ran under the ambient thread context classloader instead of the one the caller requested. Fixed by scoping the thread context classloader around eval(). A null loader leaves the caller's thread context classloader untouched, as GroovyShell falls back to its own loader for a null parent.
• OpenICFWebSocketCreator.unauthorized(): dropped the specific message it was passed and always sent the same generic rejection reason.

Also removes genuinely dead OperationOptions/typeName parameters from private helpers and updates their call sites: ActiveDirectoryChangeLogSyncStrategy.handleEvents, SchemaApiOpTests.getTestPropertyOrFail, CSVFileConnector.findAccount/doDelete/doUpdate.

Test plan

• New unit tests: ADUserAccountControlTests, JavaScriptExecutorFactoryTests, UnauthorizedResponseTest — RED before the fix, GREEN after.
• mvn install on connector-framework-internal, connector-framework-contract, connector-server-jetty, OpenICF-ldap-connector, OpenICF-csvfile-connector (incl. their existing test suites) — all green.
• Review round 1: tests pin the TCCL restore on the compiled path and after a throwing script, the null-loader case, and that unauthorized() forwards its reason with a 403. Each kills a mutant the earlier tests let through (missing finally restore, set-eval-restore without try/finally, no null guard, hard-coded reason, non-403 status). After the rebase onto current master, mvn install on connector-framework-internal (509 tests) and connector-server-jetty (36 tests) is green.

[maximthomas]

praise: The three real bugs are fixed where they live, and the new tests go red against the old code.

• ADUserAccountControl's private constructor now stores msDSUac in its own field, so isAccountLockOut()/isPasswordExpired() finally read ms-DS-User-Account-Control-Computed.
• Both JS executors restore the previous TCCL in finally (JavaScriptExecutorFactory.java:112-116, :140-144), so a throwing script does not leak the caller's loader into a pooled thread.
• UnauthorizedResponseTest fails against the base OpenICFWebSocketCreator and passes at head.

---

question (non-blocking): Should a null loader keep the ambient thread context classloader, as it did before this PR?

OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/common/script/javascript/JavaScriptExecutorFactory.java:111, :139

Both executors call setContextClassLoader(loader) without a null check, and ScriptExecutorFactory.newScriptExecutor does not forbid null. testbundlev1's TstAbstractConnector.runScriptOnResource (:375) passes null with compile=true. At head, that script runs with TCCL = null instead of the bundle loader that ThreadClassLoaderManagerProxy set. Under Rhino 1.7.15, Packages.<bundle-only class> then resolves to a package object, not the class (measured: base "function", head "object"). On the legacy connector server, whose workers are CCLWatchThreads, every such run also logs ERROR Attempting to set the CCL of thread ... to null with a stack trace (CCLWatchThread.java:56-60). This is Minor whatever the answer: no production caller passes null. In this repo only the test bundle does, and a GitHub code search over the ConnId/Evolveum/ForgeRock/WrenSecurity connectors found none. If the answer is yes, guard both executors:

// CompiledJavaScriptExecutor.execute; the same in JavaScriptExecutor.execute around engine.eval(script)
if (loader == null) {
    return compiled.eval(newContext);
}
Thread currentThread = Thread.currentThread();
ClassLoader previousLoader = currentThread.getContextClassLoader();
currentThread.setContextClassLoader(loader);
try {
    return compiled.eval(newContext);
} finally {
    currentThread.setContextClassLoader(previousLoader);
}

Pin: newScriptExecutor(null, "java.lang.Thread.currentThread().getContextClassLoader();", true).execute(null) must return the test thread's TCCL. At head it returns null.

---

suggestion (non-blocking): No test pins the TCCL restore on the compiled path or after a throwing script.

OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/common/script/javascript/JavaScriptExecutorFactoryTests.java:59-65, :51-55

The only post-execute check, testScriptDoesNotLeakClassLoaderAfterExecution, uses compile=false. testCompiledScriptRunsWithProvidedClassLoader reads the TCCL only during eval, and every test script finishes normally. Two mutants keep all 5 tests green: deleting the restore from CompiledJavaScriptExecutor.execute's finally (JavaScriptExecutorFactory.java:115), and replacing try/finally with set-eval-restore in either executor (traced, not run). Either change leaks the custom loader into the calling thread.

@Test
public void testCompiledScriptDoesNotLeakClassLoaderAfterExecution() throws Exception {
    ClassLoader before = Thread.currentThread().getContextClassLoader();
    ClassLoader custom = new URLClassLoader(new java.net.URL[0], getClass().getClassLoader());
    getScriptExecutor("1;", custom, true).execute(null);
    assertSame(Thread.currentThread().getContextClassLoader(), before);
}

@Test
public void testFailingScriptDoesNotLeakClassLoader() throws Exception {
    ClassLoader before = Thread.currentThread().getContextClassLoader();
    ClassLoader custom = new URLClassLoader(new java.net.URL[0], getClass().getClassLoader());
    try {
        getScriptExecutor("throw 'boom';", custom, true).execute(null);
        org.testng.Assert.fail("the script must throw");
    } catch (Exception expected) {
        // ScriptException from eval
    }
    assertSame(Thread.currentThread().getContextClassLoader(), before);
}

Pin: the first case kills the missing-restore mutant on the compiled path. The second kills the set-eval-restore mutant.

---

suggestion (non-blocking): UnauthorizedResponseTest does not pin that unauthorized() forwards its message, nor the 403.

OpenICF-java-framework/connector-server-jetty/src/test/java/org/forgerock/openicf/framework/server/jetty/UnauthorizedResponseTest.java:87-98

The test goes through the single caller (OpenICFWebSocketCreator.java:133), which always passes the literal "Unknown Principal". It asserts only contains("Unknown Principal"), and the sendError handler ignores a[0]. Two mutants of unauthorized() (:166) stay green: one that hardcodes "Unknown Principal: a client certificate ..." and drops message, and one with any status other than SC_FORBIDDEN (traced, not run). The test's own Javadoc promises "the specific reason it was passed".

@Test
public void testUnauthorizedForwardsTheGivenReasonWith403() throws Exception {
    ScheduledThreadPoolExecutor scheduler = new ScheduledThreadPoolExecutor(1);
    try {
        OpenICFWebSocketCreator creator = new OpenICFWebSocketCreator(null, noopListener(),
                new Authenticator() {
                    @Override
                    public void authenticate(JettyServerUpgradeRequest request,
                            JettyServerUpgradeResponse response, NameCallback callback) {
                    }
                }, scheduler);
        final Object[] sent = new Object[2];
        JettyServerUpgradeResponse response = (JettyServerUpgradeResponse) Proxy.newProxyInstance(
                UnauthorizedResponseTest.class.getClassLoader(),
                new Class<?>[] { JettyServerUpgradeResponse.class },
                new InvocationHandler() {
                    public Object invoke(Object p, Method m, Object[] a) {
                        if ("sendError".equals(m.getName())) {
                            sent[0] = a[0];
                            sent[1] = a[1];
                        }
                        return null;
                    }
                });

        creator.unauthorized(response, "Key mismatch");

        Assert.assertEquals(sent[0], 403);
        Assert.assertTrue(((String) sent[1]).startsWith("Key mismatch: "));
    } finally {
        scheduler.shutdownNow();
    }
}

Pin: a reason other than the one production literal kills the hardcoding mutant, and the sent[0] check kills the status-code one.

[vharseko]

All three points are addressed in fe0e461; the branch is also rebased onto current master.

null loader (question): yes. Both executors now leave the thread context classloader untouched when loader == null, which restores the pre-PR behaviour for TstAbstractConnector.runScriptOnResource and avoids the CCLWatchThread error log. This also matches the Groovy factory, where new GroovyShell(null) falls back to its own loader rather than failing. Pinned by testNullClassLoaderKeepsTheCallersContextClassLoader (compiled and uncompiled); it fails with the guard removed.

TCCL restore tests: added testCompiledScriptDoesNotLeakClassLoaderAfterExecution and testFailingScriptDoesNotLeakClassLoader; the latter runs on both paths via a data provider. Checked against the mutants: deleting the restore from CompiledJavaScriptExecutor's finally, and replacing try/finally with set-eval-restore in either executor — each now fails at least one test.

unauthorized() reason and status: added testUnauthorizedForwardsTheGivenReasonWith403, which calls unauthorized(response, "Key mismatch") directly and asserts both the 403 and the forwarded reason. The hard-coded-reason mutant and a 401 mutant both fail it.

mvn install on connector-framework-internal (509 tests) and connector-server-jetty (36 tests) is green.

[maximthomas]

praise: The round-1 feedback landed where it was aimed.

• Both executors now return eval() directly for a null loader (JavaScriptExecutorFactory.java:111-112, :142-143), so the caller's TCCL is kept; testNullClassLoaderKeepsTheCallersContextClassLoader goes red on the old setContextClassLoader(null).
• UnauthorizedResponseTest calls unauthorized(response, reason) directly and asserts SC_FORBIDDEN and the forwarded reason.
• testCompiledScriptDoesNotLeakClassLoaderAfterExecution and testFailingScriptDoesNotLeakClassLoader cover the compiled path and a throwing script.

[vharseko]

Rebased onto current master (88760f1, after #133 and #134) to clear the merge conflict.

The only conflict was the license header of ActiveDirectoryChangeLogSyncStrategy.java: master already has Portions Copyrighted 2026 3A Systems, LLC, so I kept that line instead of adding a second one. Compared with the previous head (fe0e461) via git range-diff, nothing else changed; the review commit applied cleanly. The whole reactor compiles with test sources (mvn test-compile -DskipTests).