[
https://issues.apache.org/jira/browse/TOMEE-4654?focusedWorklogId=1032357&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1032357
]
ASF GitHub Bot logged work on TOMEE-4654:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 27/Jul/26 08:13
Start Date: 27/Jul/26 08:13
Worklog Time Spent: 10m
Work Description: rzo1 commented on PR #2846:
URL: https://github.com/apache/tomee/pull/2846#issuecomment-5088905273
The underlying bug is real and worth fixing — the read-only machinery has
been there since
d5b3b93d4f (2017) and nothing ever enabled it, so every ENC has been
writable in violation
of EE.5.3.4 / Enterprise Beans 10.4.4. I applied the patch and confirmed
`JavaCompReadOnlyTest`
fails on unpatched main with `bind should have been refused` and passes with
the fix, and that
the read-only cascade doesn't leak into the container root context or the
per-app global/module
contexts.
I can't approve as-is though — the marking happens at the wrong point in the
lifecycle, and it
cuts both ways:
**Container-internal binds can now fail.** `Assembler.isSkip` (:1634-1645)
skips every webapp
`EjbJarInfo` when the app is not `webAppAlone`. So for an EAR containing a
WAR, that WAR's
EJB/managed-bean module is deployed later, by
`TomcatWebAppBuilder.startInternal` via
`assembler.initEjbs` (:1453) and `startEjbs` (:1468) — i.e. after
`createApplication` has already
marked `appContext.getAppJndiContext()` read-only. `initEjbs` ->
`jndiBuilder.build` ->
`JndiBuilder.bindJava` does `appContext.bind("app/" + moduleName + beanName,
ref)`
(`JndiBuilder:694`, `:722`), outside the `NameAlreadyBoundException` catch,
and the caller wraps
`NamingException` into `OpenEJBRuntimeException` (:443).
I probed this directly in openejb-core: with the patch applied, after
`createApplication` returns,
`app1.getAppJndiContext().bind("app/mod3/SomeBean", ref)` fails with
`javax.naming.OperationNotSupportedException`. I couldn't run a full EAR
deployment here, so the
end-to-end failure is inferred from the call chain — but the refusal itself
is proven and the
call chain is unconditional. Reload of such a WAR re-enters the same path
(`TomcatWebAppBuilder:2087` only destroys the app when
`isUnDeployable`/`webAppAlone`).
**And the same modules never get marked.** The flip side:
`setAppNamingContextReadOnly` only
iterates `allDeployments`, the BeanContexts built during
`createApplication`. The webapp modules
excluded by `isSkip` get their BeanContexts created afterwards, so their
`java:comp` stays fully
writable — the spec violation you're targeting survives for exactly the EJBs
that live in an EAR's
web modules.
Both problems have one shape of fix: record the read-only intent on the
`AppContext` and apply it
where the BeanContexts are actually created (end of `initEjbs`/`startEjbs`),
so late modules inherit
it and the container's own binds all run before the flag takes effect.
Other things:
- The new `WebContext` loop is dead code. Neither `TomcatWebAppBuilder` nor
`LightweightWebAppBuilder` stores an `IvmContext` or `ContextHandler` in
`WebContext.jndiEnc`, so
`markReadOnly(webContext.getJndiEnc())` never matches either branch. That
means the PR body's
claim that this is what fixed the web vehicles isn't supported by the
code, and web components
still get a writable ENC.
- `arquillian-tomee-embedded`'s
`EmbeddedTomEEContainerTest.testEjbCanCreateSubContextByDefault`
(:72-88) still asserts the pre-PR semantic. It needs to be inverted in
this PR, and honestly a
container-wide default flip with this blast radius shouldn't land with
only openejb-core unit
coverage — an Arquillian test against a real TomEE would be worth it.
- The opt-out is read from `SystemInstance` only, never from
`appInfo.properties`. Most other
Assembler switches support both (e.g. `OPENEJB_TIMERS_ON` at :1501/:1560
reads
`appInfo.properties.getProperty(..., globalDefault)`). As written, one
legacy app that writes into
its ENC forces the whole container off the spec-required behaviour.
Reading `appInfo.properties`
first with the system property as fallback keeps it per-application.
- `assertWriteRefused` hard-requires `OperationNotSupportedException`, which
couples the new test to
`openejb.jndiExceptionOnFailedWrite` — the `createSubcontext` case right
below it correctly
tolerates both outcomes. Make them consistent.
- Minor test things: the `rename`/`destroySubcontext` assertions are
tautological as written;
`deploy()` runs outside the try/finally so a failure there skips
`SystemInstance.reset()` and
pollutes the next test.
-
`"true".equals(SystemInstance.get().getProperty(FORCE_READ_ONLY_APP_NAMING,
"true"))` is
case-sensitive, so `-Dopenejb.forceReadOnlyAppNamingContext=FALSE`
silently keeps read-only on.
Now that this is the opt-out for a spec-behaviour change, that matters
more than it did.
- `if(`/`for(` without the space doesn't match the file's convention.
Pre-existing in the lines you
moved, but the new `markReadOnly` helper is new code.
Issue Time Tracking
-------------------
Worklog Id: (was: 1032357)
Time Spent: 20m (was: 10m)
> java:comp naming context accepts writes the Enterprise Beans spec requires it
> to refuse
> ---------------------------------------------------------------------------------------
>
> Key: TOMEE-4654
> URL: https://issues.apache.org/jira/browse/TOMEE-4654
> Project: TomEE
> Issue Type: Bug
> Reporter: Markus Jung
> Assignee: Markus Jung
> Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> The Enterprise Beans spec requires the java:comp context to be read-only
> inside a bean, and to throw {{OperationNotSupportedException}} on any write
> attempt against it. TomEE's naming context skips that check and lets bind,
> rebind, and rename calls through instead of refusing them.
> h2. Steps to reproduce / TCK reference
> Excluded in {{runner-webprofile/exclusions/enterprise-beans-30.txt}} in the
> apache/tomee-tck harness repo:
> * {{**/naming/context/*Test.java}}
> Once fixed, remove the matching lines and confirm the tests pass — each write
> attempt against java:comp must throw {{OperationNotSupportedException}}.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
