SLING-7805 - NPE in Oak SessionImpl when starting up
authorRobert Munteanu <rombert@apache.org>
Wed, 8 Aug 2018 12:15:04 +0000 (15:15 +0300)
committerRobert Munteanu <rombert@apache.org>
Wed, 8 Aug 2018 12:15:04 +0000 (15:15 +0300)
This reverts commit 335d0aec54d8674b275b51286a18fff92e0eaccc, reversing
changes made to bf18d14584cacbadcaec443821433e78669a43a6.

It also reverts commits 5a36b33e05b1b05827993ad09bd4e389674cd8a1 and
dde168f8e1c205a04a972b8b1997c002b41055d3.

pom.xml
src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrProviderStateFactory.java
src/test/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProviderSessionHandlingTest.java [deleted file]
src/test/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProviderTest.java

diff --git a/pom.xml b/pom.xml
index b1059c9..1d18f59 100644 (file)
--- a/pom.xml
+++ b/pom.xml
@@ -54,7 +54,6 @@
             <plugin>
                 <groupId>org.apache.sling</groupId>
                 <artifactId>maven-sling-plugin</artifactId>
-                <version>2.3.2</version>
                 <executions>
                     <execution>
                         <id>generate-adapter-metadata</id>
         <dependency>
             <groupId>org.apache.sling</groupId>
             <artifactId>org.apache.sling.api</artifactId>
-            <version>2.18.2</version>
+            <version>2.16.4</version>
             <scope>provided</scope>
         </dependency>
         <dependency>
index 2f43526..8767fa9 100644 (file)
@@ -145,12 +145,7 @@ public class JcrProviderStateFactory {
             @Nonnull final Map<String, Object> authenticationInfo,
             @Nullable final BundleContext ctx
     ) throws LoginException {
-        boolean explicitSessionUsed = (getSession(authenticationInfo) != null);
-        final Session impersonatedSession = handleImpersonation(session, authenticationInfo, logoutSession, explicitSessionUsed);
-        if (impersonatedSession != session && explicitSessionUsed) {
-            // update the session in the auth info map in case the resolver gets cloned in the future
-            authenticationInfo.put(JcrResourceConstants.AUTHENTICATION_INFO_SESSION, impersonatedSession);
-        }
+        final Session impersonatedSession = handleImpersonation(session, authenticationInfo, logoutSession);
         // if we're actually impersonating, we're responsible for closing the session we've created, regardless
         // of what the original logoutSession value was.
         boolean doLogoutSession = logoutSession || (impersonatedSession != session);
@@ -172,35 +167,25 @@ public class JcrProviderStateFactory {
      * @param logoutSession
      *            whether to logout the <code>session</code> after impersonation
      *            or not.
-     * @param explicitSessionUsed
-     *            whether the JCR session was explicitly given in the auth info or not.
      * @return The original session or impersonated session.
      * @throws LoginException
      *             If something goes wrong.
      */
     private static Session handleImpersonation(final Session session, final Map<String, Object> authenticationInfo,
-            final boolean logoutSession, boolean explicitSessionUsed) throws LoginException {
+            final boolean logoutSession) throws LoginException {
         final String sudoUser = getSudoUser(authenticationInfo);
-        // Do we need session.impersonate() because we are asked to impersonate another user?
-        boolean needsSudo = (sudoUser != null) && !session.getUserID().equals(sudoUser);
-        // Do we need session.impersonate() to get an independent copy of the session we were given in the auth info?
-        boolean needsCloning = !needsSudo && explicitSessionUsed && authenticationInfo.containsKey(ResourceProvider.AUTH_CLONE);
-        try {
-            if (needsSudo) {
-                SimpleCredentials creds = new SimpleCredentials(sudoUser, new char[0]);
+        if (sudoUser != null && !session.getUserID().equals(sudoUser)) {
+            try {
+                final SimpleCredentials creds = new SimpleCredentials(sudoUser, new char[0]);
                 copyAttributes(creds, authenticationInfo);
                 creds.setAttribute(ResourceResolver.USER_IMPERSONATOR, session.getUserID());
                 return session.impersonate(creds);
-            } else if (needsCloning) {
-                SimpleCredentials creds = new SimpleCredentials(session.getUserID(), new char[0]);
-                copyAttributes(creds, authenticationInfo);
-                return session.impersonate(creds);
-            }
-        } catch (final RepositoryException re) {
-            throw getLoginException(re);
-        } finally {
-            if (logoutSession) {
-                session.logout();
+            } catch (final RepositoryException re) {
+                throw getLoginException(re);
+            } finally {
+                if (logoutSession) {
+                    session.logout();
+                }
             }
         }
         return session;
diff --git a/src/test/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProviderSessionHandlingTest.java b/src/test/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProviderSessionHandlingTest.java
deleted file mode 100644 (file)
index a471446..0000000
+++ /dev/null
@@ -1,199 +0,0 @@
-/*
- * 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.sling.jcr.resource.internal.helper.jcr;
-
-import static org.hamcrest.Matchers.is;
-import static org.hamcrest.Matchers.not;
-import static org.hamcrest.Matchers.sameInstance;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNotSame;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assume.assumeThat;
-import static org.junit.Assume.assumeTrue;
-import static org.mockito.Matchers.anyString;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
-
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-
-import javax.jcr.Session;
-
-import org.apache.sling.api.resource.ResourceResolver;
-import org.apache.sling.api.resource.ResourceResolverFactory;
-import org.apache.sling.commons.testing.jcr.RepositoryProvider;
-import org.apache.sling.jcr.api.SlingRepository;
-import org.apache.sling.jcr.resource.api.JcrResourceConstants;
-import org.apache.sling.spi.resource.provider.ResolveContext;
-import org.apache.sling.spi.resource.provider.ResourceProvider;
-import org.junit.After;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-import org.junit.runners.Parameterized.Parameter;
-import org.junit.runners.Parameterized.Parameters;
-import org.mockito.Mockito;
-import org.osgi.framework.ServiceReference;
-import org.osgi.service.component.ComponentContext;
-
-@RunWith(Parameterized.class)
-public class JcrResourceProviderSessionHandlingTest {
-
-    private enum LoginStyle {USER, SESSION};
-
-    private static final String AUTH_USER = "admin";
-    private static final char[] AUTH_PASSWORD = "admin".toCharArray();
-    private static final String SUDO_USER = "anonymous";
-
-    @Parameters(name = "loginStyle= {0}, sudo = {1}, clone = {2}")
-    public static List<Object[]> data() {
-
-        LoginStyle[] loginStyles = LoginStyle.values();
-        boolean[] sudoOptions = new boolean[] {false, true};
-        boolean[] cloneOptions = new boolean[] {false, true};
-
-        // Generate all possible combinations into data.
-        List<Object[]> data = new ArrayList<>();
-        Object[] dataPoint = new Object[3];
-        for (LoginStyle loginStyle : loginStyles) {
-            dataPoint[0] = loginStyle;
-            for (boolean sudo : sudoOptions) {
-                dataPoint[1] = sudo;
-                for (boolean clone : cloneOptions) {
-                    dataPoint[2] = clone;
-                    data.add(dataPoint.clone());
-                }
-            }
-        }
-        return data;
-    }
-
-    @Parameter(0)
-    public LoginStyle loginStyle;
-
-    @Parameter(1)
-    public boolean useSudo;
-
-    @Parameter(2)
-    public boolean doClone;
-
-    // Session we're using when loginStyle == SESSION, null otherwise.
-    private Session explicitSession;
-
-    private JcrResourceProvider jcrResourceProvider;
-    private JcrProviderState jcrProviderState;
-
-    @Before
-    public void setUp() throws Exception {
-        SlingRepository repo = RepositoryProvider.instance().getRepository();
-        Map<String, Object> authInfo = new HashMap<>();
-        switch (loginStyle) {
-        case USER:
-            authInfo.put(ResourceResolverFactory.USER, AUTH_USER);
-            authInfo.put(ResourceResolverFactory.PASSWORD, AUTH_PASSWORD);
-            break;
-        case SESSION:
-            explicitSession = repo.loginAdministrative(null);
-            authInfo.put(JcrResourceConstants.AUTHENTICATION_INFO_SESSION, explicitSession);
-            break;
-        }
-
-        if (useSudo) {
-            authInfo.put(ResourceResolverFactory.USER_IMPERSONATION, SUDO_USER);
-        }
-
-        if (doClone) {
-            authInfo.put(ResourceProvider.AUTH_CLONE, true);
-        }
-
-        ComponentContext ctx = mock(ComponentContext.class);
-        when(ctx.locateService(anyString(), Mockito.<ServiceReference<Object>>any())).thenReturn(repo);
-
-        jcrResourceProvider = new JcrResourceProvider();
-        jcrResourceProvider.activate(ctx);
-
-        jcrProviderState = jcrResourceProvider.authenticate(authInfo);
-    }
-
-    @After
-    public void tearDown() throws Exception {
-
-        // Some tests do a logout, so check for liveness before trying to log out.
-        if (jcrProviderState.getSession().isLive()) {
-            jcrResourceProvider.logout(jcrProviderState);
-        }
-
-        jcrResourceProvider.deactivate();
-
-        if (explicitSession != null) {
-            explicitSession.logout();
-        }
-    }
-
-    @Test
-    public void sessionUsesCorrectUser() {
-        String expectedUser = useSudo ? SUDO_USER : AUTH_USER;
-        assertEquals(expectedUser, jcrProviderState.getSession().getUserID());
-    }
-
-    @Test
-    public void explicitSessionNotClosedOnLogout() {
-        assumeTrue(loginStyle == LoginStyle.SESSION);
-
-        jcrResourceProvider.logout(jcrProviderState);
-
-        assertTrue(explicitSession.isLive());
-    }
-
-    @Test
-    public void sessionsDoNotLeak() {
-        // This test is only valid if we either didn't pass an explicit session,
-        // or the provider had to clone it. Sessions created by the provider
-        // must be closed by the provider, or we have a session leak.
-        assumeThat(jcrProviderState.getSession(), is(not(sameInstance(explicitSession))));
-
-        jcrResourceProvider.logout(jcrProviderState);
-
-        assertFalse(jcrProviderState.getSession().isLive());
-    }
-
-    @Test
-    public void impersonatorIsReportedCorrectly() {
-        assumeTrue(useSudo);
-
-        @SuppressWarnings("unchecked")
-        ResolveContext<JcrProviderState> mockContext = mock(ResolveContext.class);
-        when(mockContext.getProviderState()).thenReturn(jcrProviderState);
-        Object reportedImpersonator = jcrResourceProvider.getAttribute(mockContext, ResourceResolver.USER_IMPERSONATOR);
-
-        assertEquals(AUTH_USER, reportedImpersonator);
-    }
-
-    @Test
-    public void clonesAreIndependent() {
-        assumeTrue(loginStyle == LoginStyle.SESSION && doClone);
-
-        assertNotSame(explicitSession, jcrProviderState.getSession());
-    }
-
-}
index 32c6bfb..aa40162 100644 (file)
 package org.apache.sling.jcr.resource.internal.helper.jcr;
 
 import java.security.Principal;
+import java.util.HashMap;
+import java.util.Map;
 
 import javax.jcr.Repository;
+import javax.jcr.RepositoryException;
 import javax.jcr.Session;
+import javax.naming.NamingException;
 
+import org.apache.sling.api.resource.LoginException;
+import org.apache.sling.api.resource.ResourceResolverFactory;
 import org.apache.sling.commons.testing.jcr.RepositoryTestBase;
+import org.apache.sling.jcr.resource.api.JcrResourceConstants;
 import org.apache.sling.spi.resource.provider.ResolveContext;
 import org.junit.Assert;
 import org.mockito.Mockito;
@@ -40,24 +47,34 @@ public class JcrResourceProviderTest extends RepositoryTestBase {
         super.setUp();
         // create the session
         session = getSession();
-        Repository repo = getRepository();
-        ComponentContext ctx = Mockito.mock(ComponentContext.class);
-        Mockito.when(ctx.locateService(Mockito.anyString(), Mockito.any(ServiceReference.class))).thenReturn(repo);
-        jcrResourceProvider = new JcrResourceProvider();
-        jcrResourceProvider.activate(ctx);
     }
 
     @Override
     protected void tearDown() throws Exception {
-        jcrResourceProvider.deactivate();
         super.tearDown();
     }
 
     public void testAdaptTo_Principal() {
+        jcrResourceProvider = new JcrResourceProvider();
         ResolveContext ctx = Mockito.mock(ResolveContext.class);
         Mockito.when(ctx.getProviderState()).thenReturn(new JcrProviderState(session, null, false));
         Assert.assertNotNull(jcrResourceProvider.adaptTo(ctx, Principal.class));
     }
+    
+    public void testLeakOnSudo() throws LoginException, RepositoryException, NamingException {
+        Repository repo = getRepository();
+        ComponentContext ctx = Mockito.mock(ComponentContext.class);
+        Mockito.when(ctx.locateService(Mockito.anyString(), Mockito.any(ServiceReference.class))).thenReturn(repo);
+        jcrResourceProvider = new JcrResourceProvider();
+        jcrResourceProvider.activate(ctx);
+        Map<String, Object> authInfo = new HashMap<String, Object>();
+        authInfo.put(JcrResourceConstants.AUTHENTICATION_INFO_SESSION, session);
+        authInfo.put(ResourceResolverFactory.USER_IMPERSONATION, "anonymous");
+        JcrProviderState providerState = jcrResourceProvider.authenticate(authInfo);
+        Assert.assertNotEquals("Impersonation didn't start new session", session, providerState.getSession());
+        jcrResourceProvider.logout(providerState);
+        assertFalse("Impersonated session wasn't closed.", providerState.getSession().isLive());
+    }
 }