TOMEE-4654 - Make the component naming context read-only by default

container/openejb-core/src/main/java/org/apache/openejb/assembler/classic/Assembler.java

@@ -1094,7 +1094,7 @@ private AppContext createApplication(final AppInfo appInfo, ClassLoader classLoa
                 logger.info("createApplication.success", appInfo.path);
 
                 //required by spec EE.5.3.4
-                if(setAppNamingContextReadOnly(allDeployments)) {
+                if(setAppNamingContextReadOnly(appContext, allDeployments)) {
                     logger.info("createApplication.naming", appInfo.path);
                 }
 
@@ -1117,22 +1117,33 @@ private AppContext createApplication(final AppInfo appInfo, ClassLoader classLoa
         }
     }
 
-    boolean setAppNamingContextReadOnly(final List<BeanContext> allDeployments) {
-        if("true".equals(SystemInstance.get().getProperty(FORCE_READ_ONLY_APP_NAMING, "false"))) {
+    boolean setAppNamingContextReadOnly(final AppContext appContext, final List<BeanContext> allDeployments) {
+        if("true".equals(SystemInstance.get().getProperty(FORCE_READ_ONLY_APP_NAMING, "true"))) {
             for(BeanContext beanContext : allDeployments) {
-                Context ctx = beanContext.getJndiContext();
-
-                if(IvmContext.class.isInstance(ctx)) {
-                    IvmContext.class.cast(ctx).setReadOnly(true);
-                } else if(ContextHandler.class.isInstance(ctx)) {
-                    ContextHandler.class.cast(ctx).setReadOnly();
+                markReadOnly(beanContext.getJndiContext());
+            }
+
+            // servlets, JSF beans and other web components resolve java:comp/java:module/java:app through the
+            // web and app contexts rather than through a BeanContext, so they need the same treatment
+            if(appContext != null) {
+                for(final WebContext webContext : appContext.getWebContexts()) {
+                    markReadOnly(webContext.getJndiEnc());
                 }
+                markReadOnly(appContext.getAppJndiContext());
             }
             return true;
         }
         return false;
     }
 
+    private static void markReadOnly(final Context ctx) {
+        if(IvmContext.class.isInstance(ctx)) {
+            IvmContext.class.cast(ctx).setReadOnly(true);
+        } else if(ContextHandler.class.isInstance(ctx)) {
+            ContextHandler.class.cast(ctx).setReadOnly();
+        }
+    }
+
     private List<String> getDuplicates(final AppInfo appInfo) {
         final List<String> used = new ArrayList<>();
         for (final EjbJarInfo ejbJarInfo : appInfo.ejbJars) {

container/openejb-core/src/test/java/org/apache/openejb/assembler/classic/AppNamingReadOnlyTest.java

@@ -43,10 +43,10 @@ public void testReadOnlyAppNamingContext() throws SystemException, URISyntaxExce
         System.setProperty(Assembler.FORCE_READ_ONLY_APP_NAMING, Boolean.TRUE.toString());
         try {
         	List<BeanContext> mockBeanContextsList = getMockBeanContextsList();
-        	
+
         	Assembler assembler = new Assembler();
-        	assembler.setAppNamingContextReadOnly(mockBeanContextsList);
-        	
+        	assembler.setAppNamingContextReadOnly(null, mockBeanContextsList);
+
         	Context beanNamingContext = mockBeanContextsList.get(0).getJndiContext();
         	//may return null or throw exception depending on openejb.jndiExceptionOnFailedWrite value;
         	//this test is not intended to test read-only behavior (null/exception); it should check whether naming context is marked as read only 
@@ -68,18 +68,45 @@ public void testReadOnlyAppNamingContext() throws SystemException, URISyntaxExce
         }
     }
 
-    //check TOMEE behavior is backward compatible
-    public void testAppNamingContextWritableByDefault() throws SystemException, URISyntaxException, NamingException {
+    //read-only is the spec-mandated default (EE.5.3.4, Enterprise Beans 10.4.4)
+    public void testAppNamingContextReadOnlyByDefault() throws SystemException, URISyntaxException {
 
-    	List<BeanContext> mockBeanContextsList = getMockBeanContextsList();
-
-    	Assembler assembler = new Assembler();
-    	assembler.setAppNamingContextReadOnly(mockBeanContextsList);
-
-    	Context beanNamingContext = mockBeanContextsList.get(0).getJndiContext();
-		Context subContext = beanNamingContext.createSubcontext("sub");
-
-		assertNotNull(subContext);
+        List<BeanContext> mockBeanContextsList = getMockBeanContextsList();
+
+        Assembler assembler = new Assembler();
+        assertTrue(assembler.setAppNamingContextReadOnly(null, mockBeanContextsList));
+
+        Context beanNamingContext = mockBeanContextsList.get(0).getJndiContext();
+        try {
+            assertNull(beanNamingContext.createSubcontext("sub"));
+        } catch (OperationNotSupportedException e) {
+            //ok
+        } catch (NamingException e) {
+            throw new AssertionError(e);
+        }
+    }
+
+    //the legacy writable behavior is still available as an explicit opt-out
+    public void testAppNamingContextWritableWhenDisabled() throws SystemException, URISyntaxException, NamingException {
+
+        String originalValue = System.getProperty(Assembler.FORCE_READ_ONLY_APP_NAMING);
+        System.setProperty(Assembler.FORCE_READ_ONLY_APP_NAMING, Boolean.FALSE.toString());
+        try {
+            List<BeanContext> mockBeanContextsList = getMockBeanContextsList();
+
+            Assembler assembler = new Assembler();
+            assertFalse(assembler.setAppNamingContextReadOnly(null, mockBeanContextsList));
+
+            Context beanNamingContext = mockBeanContextsList.get(0).getJndiContext();
+            assertNotNull(beanNamingContext.createSubcontext("sub"));
+        } finally {
+            if(originalValue == null) {
+                System.clearProperty(Assembler.FORCE_READ_ONLY_APP_NAMING);
+            } else {
+                System.setProperty(Assembler.FORCE_READ_ONLY_APP_NAMING, originalValue);
+            }
+            SystemInstance.reset();
+        }
     }
 
     private List<BeanContext> getMockBeanContextsList() throws SystemException, URISyntaxException {

container/openejb-core/src/test/java/org/apache/openejb/core/ivm/naming/JavaCompReadOnlyTest.java

@@ -0,0 +1,123 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ */
+package org.apache.openejb.core.ivm.naming;
+
+import junit.framework.TestCase;
+import org.apache.openejb.AppContext;
+import org.apache.openejb.BeanContext;
+import org.apache.openejb.assembler.classic.Assembler;
+import org.apache.openejb.assembler.classic.SecurityServiceInfo;
+import org.apache.openejb.assembler.classic.TransactionServiceInfo;
+import org.apache.openejb.config.AppModule;
+import org.apache.openejb.config.ConfigurationFactory;
+import org.apache.openejb.config.EjbModule;
+import org.apache.openejb.jee.EjbJar;
+import org.apache.openejb.jee.SingletonBean;
+import org.apache.openejb.loader.SystemInstance;
+
+import javax.naming.Context;
+import javax.naming.OperationNotSupportedException;
+
+/**
+ * The Enterprise Beans spec (10.4.4) and EE.5.3.4 require the component naming context to be read-only:
+ * writes against java:comp and friends must not take effect.
+ */
+public class JavaCompReadOnlyTest extends TestCase {
+
+    private AppContext deploy() throws Exception {
+        final ConfigurationFactory config = new ConfigurationFactory();
+        final Assembler assembler = new Assembler();
+
+        assembler.createTransactionManager(config.configureService(TransactionServiceInfo.class));
+        assembler.createSecurityService(config.configureService(SecurityServiceInfo.class));
+
+        final EjbJar ejbJar = new EjbJar("testmodule");
+        ejbJar.addEnterpriseBean(new SingletonBean(Bean.class));
+
+        final AppModule module = new AppModule(new EjbModule(ejbJar));
+        return assembler.createApplication(config.configureApplication(module));
+    }
+
+    public void testCompContextRefusesWrites() throws Exception {
+        final AppContext app = deploy();
+        try {
+            final BeanContext bean = app.getBeanContexts().get(0);
+            final Context comp = bean.getJndiContext();
+
+            assertWriteRefused(comp, "bind", () -> comp.bind("newName", "newValue"));
+            assertWriteRefused(comp, "rebind", () -> comp.rebind("newName", "newValue"));
+            assertWriteRefused(comp, "rename", () -> comp.rename("comp", "renameTo"));
+            assertWriteRefused(comp, "unbind", () -> comp.unbind("comp"));
+            assertWriteRefused(comp, "destroySubcontext", () -> comp.destroySubcontext("comp"));
+
+            // createSubcontext either throws or returns null, depending on jndiExceptionOnFailedWrite
+            try {
+                assertNull(comp.createSubcontext("newName"));
+            } catch (final OperationNotSupportedException expected) {
+                // ok
+            }
+
+            // nothing the writes attempted may be observable afterwards
+            assertNotBound(comp, "newName");
+            assertNotBound(comp, "renameTo");
+
+            // and the pre-existing binding must have survived unbind/rename/destroySubcontext
+            assertTrue(comp.lookup("comp") instanceof Context);
+        } finally {
+            SystemInstance.reset();
+        }
+    }
+
+    public void testAppContextRefusesWrites() throws Exception {
+        final AppContext app = deploy();
+        try {
+            final Context appCtx = app.getAppJndiContext();
+
+            assertWriteRefused(appCtx, "bind", () -> appCtx.bind("newName", "newValue"));
+            assertNotBound(appCtx, "newName");
+            assertTrue(appCtx.lookup("app") instanceof Context);
+        } finally {
+            SystemInstance.reset();
+        }
+    }
+
+    private interface Write {
+        void run() throws Exception;
+    }
+
+    private void assertWriteRefused(final Context ctx, final String operation, final Write write) {
+        try {
+            write.run();
+            fail(operation + " should have been refused on a read-only naming context");
+        } catch (final OperationNotSupportedException expected) {
+            // ok
+        } catch (final Exception e) {
+            throw new AssertionError("unexpected exception from " + operation, e);
+        }
+    }
+
+    private void assertNotBound(final Context ctx, final String name) throws Exception {
+        try {
+            assertNull(name + " must not be bound", ctx.lookup(name));
+        } catch (final javax.naming.NameNotFoundException expected) {
+            // ok
+        }
+    }
+
+    public static class Bean {
+    }
+}