negative cache

fixes #3859

Author: vladak

plugins/src/main/java/opengrok/auth/entity/LdapUser.java

@@ -18,7 +18,7 @@
  */
 
 /*
- * Copyright (c) 2017, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2022, Oracle and/or its affiliates. All rights reserved.
  */
 package opengrok.auth.entity;
 
@@ -46,7 +46,6 @@ public LdapUser() {
 
     public LdapUser(String dn, Map<String, Set<String>> attrs) {
         this.dn = dn;
-
         this.attributes = Objects.requireNonNullElseGet(attrs, HashMap::new);
     }
 

plugins/src/main/java/opengrok/auth/plugin/LdapUserPlugin.java

@@ -23,6 +23,7 @@
 package opengrok.auth.plugin;
 
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
@@ -35,6 +36,7 @@
 import opengrok.auth.plugin.entity.User;
 import opengrok.auth.plugin.ldap.AbstractLdapProvider;
 import opengrok.auth.plugin.ldap.LdapException;
+import org.jetbrains.annotations.NotNull;
 import org.opengrok.indexer.authorization.AuthorizationException;
 import org.opengrok.indexer.configuration.Group;
 import org.opengrok.indexer.configuration.Project;
@@ -52,6 +54,7 @@ public class LdapUserPlugin extends AbstractLdapPlugin {
     private static final Logger LOGGER = Logger.getLogger(LdapUserPlugin.class.getName());
 
     static final String SESSION_ATTR = "opengrok-ldap-plugin-user";
+    static final String NEGATIVE_CACHE_ATTR = "opengrok-ldap-plugin-user-invalid-user";
 
     /**
      * List of configuration names.
@@ -135,9 +138,7 @@ protected boolean sessionExists(HttpServletRequest req) {
      */
     String expandFilter(User user) {
         String filter = ldapFilter;
-
         filter = expandUserFilter(user, filter);
-
         filter = filter.replace("\\%", "%");
 
         return filter;
@@ -170,19 +171,20 @@ public void fillSession(HttpServletRequest req, User user) {
         AbstractLdapProvider ldapProvider = getLdapProvider();
         try {
             AbstractLdapProvider.LdapSearchResult<Map<String, Set<String>>> res;
-            if ((res = ldapProvider.lookupLdapContent(dn, expandedFilter,
-                    attrSet.toArray(new String[0]))) == null) {
+            if ((res = ldapProvider.lookupLdapContent(dn, expandedFilter, attrSet.toArray(new String[0]))) == null) {
                 LOGGER.log(Level.WARNING, "failed to get LDAP attributes ''{2}'' for user {0} " +
                                 "with filter ''{1}'' from LDAP provider {3}",
                         new Object[]{user, expandedFilter, attrSet, getLdapProvider()});
+                LdapUser ldapUser = new LdapUser(dn, null);
+                ldapUser.setAttribute(NEGATIVE_CACHE_ATTR, Collections.singleton(null));
+                updateSession(req, ldapUser);
                 return;
             }
 
             records = res.getAttrs();
             if (Boolean.FALSE.equals(useDN)) {
                 dn = res.getDN();
-                LOGGER.log(Level.FINEST, "got DN ''{0}'' for user {1}",
-                        new Object[]{dn, user});
+                LOGGER.log(Level.FINEST, "got DN ''{0}'' for user {1}", new Object[]{dn, user});
             }
         } catch (LdapException ex) {
             throw new AuthorizationException(ex);
@@ -206,8 +208,7 @@ public void fillSession(HttpServletRequest req, User user) {
             userAttrSet.put(attrName, records.get(attrName));
         }
 
-        LOGGER.log(Level.FINEST, "DN for user {0} is ''{1}'' on {2}",
-                new Object[]{user, dn, ldapProvider});
+        LOGGER.log(Level.FINEST, "DN for user {0} is ''{1}'' on {2}", new Object[]{user, dn, ldapProvider});
         updateSession(req, new LdapUser(dn, userAttrSet));
     }
 
@@ -217,7 +218,7 @@ public void fillSession(HttpServletRequest req, User user) {
      * @param req the request
      * @param user the new value for user
      */
-    void updateSession(HttpServletRequest req, LdapUser user) {
+    void updateSession(@NotNull HttpServletRequest req, LdapUser user) {
         req.getSession().setAttribute(getSessionAttrName(), user);
     }
 
@@ -229,13 +230,18 @@ private String getSessionAttrName() {
         return getSessionAttrName(instanceNum);
     }
 
+    private boolean checkUser(@NotNull HttpServletRequest request) {
+        LdapUser ldapUser = (LdapUser) request.getSession().getAttribute(getSessionAttrName());
+        return ldapUser != null && ldapUser.getAttribute(NEGATIVE_CACHE_ATTR) == null;
+    }
+
     @Override
     public boolean checkEntity(HttpServletRequest request, Project project) {
-        return request.getSession().getAttribute(getSessionAttrName()) != null;
+        return checkUser(request);
     }
 
     @Override
     public boolean checkEntity(HttpServletRequest request, Group group) {
-        return request.getSession().getAttribute(getSessionAttrName()) != null;
+        return checkUser(request);
     }
 }

plugins/src/test/java/opengrok/auth/plugin/LdapUserPluginTest.java

@@ -37,15 +37,24 @@
 import opengrok.auth.plugin.util.DummyHttpServletRequestLdap;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+import org.opengrok.indexer.configuration.Group;
+import org.opengrok.indexer.configuration.Project;
 
 import static opengrok.auth.plugin.LdapUserPlugin.SESSION_ATTR;
-import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.ArgumentMatchers.isNull;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 /**
@@ -111,7 +120,7 @@ void testFillSessionWithDnOff() throws LdapException {
         params.put(LdapUserPlugin.USE_DN, false);
         LdapUserPlugin plugin = new LdapUserPlugin();
         plugin.load(params, mockprovider);
-        assertEquals(mockprovider, plugin.getLdapProvider());
+        assertSame(mockprovider, plugin.getLdapProvider());
 
         HttpServletRequest request = new DummyHttpServletRequestLdap();
         User user = new User("[email protected]", "id");
@@ -121,6 +130,31 @@ void testFillSessionWithDnOff() throws LdapException {
         assertEquals(dn, ((LdapUser) request.getSession().getAttribute(SESSION_ATTR)).getDn());
     }
 
+    @Test
+    void testNegativeCache() throws LdapException {
+        AbstractLdapProvider mockprovider = mock(LdapFacade.class);
+        when(mockprovider.lookupLdapContent(isNull(), isNull(), any(String[].class))).thenReturn(null);
+
+        Map<String, Object> params = getParamsMap();
+        params.put(LdapUserPlugin.ATTRIBUTES, "mail");
+        params.put(LdapUserPlugin.USE_DN, false);
+        LdapUserPlugin origPlugin = new LdapUserPlugin();
+        LdapUserPlugin plugin = Mockito.spy(origPlugin);
+        plugin.load(params, mockprovider);
+        assertSame(mockprovider, plugin.getLdapProvider());
+
+        HttpServletRequest dummyRequest = new DummyHttpServletRequestLdap();
+        User user = new User("[email protected]", "id");
+        dummyRequest.setAttribute(UserPlugin.REQUEST_ATTR, new User("foo", "123"));
+        plugin.fillSession(dummyRequest, user);
+
+        assertNotNull(dummyRequest.getSession().getAttribute(SESSION_ATTR));
+        assertFalse(plugin.isAllowed(dummyRequest, new Project("foo")));
+        assertFalse(plugin.isAllowed(dummyRequest, new Group("bar")));
+        // Make sure that the session was filled so that the second call to isAllowed() did not fill it again.
+        verify(plugin, times(2)).updateSession(eq(dummyRequest), anyString(), anyBoolean());
+    }
+
     @Test
     void testInstance() {
         Map<String, Object> params = getParamsMap();