HPCC-17279 Improve LDAP logging

There are numerous error paths in LDAP security manager that don't log
anything. This makes it difficult for users to determine their problems
without getting platform developers involved, and makes it hard for those
developers to do the same. This PR adds additional logging to help trace
these errors

Signed-off-by: Russ Whitehead <william.whitehead@lexisnexis.com>

system/security/LdapSecurity/ldapconnection.cpp

@@ -1475,32 +1475,16 @@ public:
             const char* username = user.getName();
             const char* password = user.credentials().getPassword();
             if(!username || !*username || !password || !*password)
+            {
+                DBGLOG("CLdapClient::authenticate username/password must be provided");
                 return false;
+            }
 
             if (getMaxPwdAge(m_connections,(char*)m_ldapconfig->getBasedn(), m_ldapconfig->getLdapTimeout()) != PWD_NEVER_EXPIRES)
                 m_domainPwdsNeverExpire = false;
             else
                 m_domainPwdsNeverExpire = true;
 
-            const char* sysuser = m_ldapconfig->getSysUser();
-            bool sysUser = false;
-            if(sysuser && *sysuser && (strcmp(username, sysuser) == 0))
-            {
-                if(strcmp(password, m_ldapconfig->getSysUserPassword()) == 0)
-                {
-                    user.setFullName(m_ldapconfig->getSysUserCommonName());
-                    user.setAuthenticateStatus(AS_AUTHENTICATED);
-                    if (m_ldapconfig->getServerType() != ACTIVE_DIRECTORY)
-                        return true;
-                    sysUser = true;
-                }
-                else
-                {
-                    user.setAuthenticateStatus(AS_INVALID_CREDENTIALS);
-                    return false;
-                }
-            }
-
             StringBuffer filter;
             // Retrieve user's dn with system connection
             if(m_ldapconfig->getServerType() == ACTIVE_DIRECTORY)
@@ -1638,9 +1622,6 @@ public:
             userdnbuf.append(userdn);
             ldap_memfree(userdn);
 
-            if (sysUser)
-                return true;//sysuser authenticated above
-
             StringBuffer hostbuf;
             m_ldapconfig->getLdapHost(hostbuf);
             int rc = LDAP_SERVER_DOWN;
@@ -1741,7 +1722,10 @@ public:
 
         const char* username = user.getName();
         if(!username || !*username)
+        {
+            DBGLOG("CLdapClient::authorize username must be specified");
             return false;
+        }
 
         const char* sysuser = m_ldapconfig->getSysUser();
         if(sysuser && *sysuser && (strcmp(username, sysuser) == 0))
@@ -2246,7 +2230,7 @@ public:
             {
                 searchResult.ldapMsgFree();
                 rc = ldap_search_ext_s(ld, (char*)m_ldapconfig->getSysUserBasedn(), LDAP_SCOPE_SUBTREE, (char*)filter.str(), attrs, 0, NULL, NULL, &timeOut, LDAP_NO_LIMIT, &searchResult.msg );
-                //DBGLOG("No entries are found");
+                DBGLOG("CLdapClient::lookupAccount No entries found");
                 return false;
             }
         }
@@ -2545,7 +2529,10 @@ public:
     {
         const char* usrName = usr.getName();
         if(!usrName || !*usrName)
+        {
+            DBGLOG("CLdapClient::addUserTree username must be provided");
             return;
+        }
 
         const char* fullName = usr.getFullName();
         StringBuffer sb;
@@ -2647,7 +2634,10 @@ public:
     {
         const char* username = user.getName();
         if(!username || !*username)
+        {
+            DBGLOG("CLdapClient::updateUser username must be provided");
             return false;
+        }
 
         StringBuffer userdn;
         getUserDN(username, userdn);
@@ -3190,7 +3180,10 @@ public:
     {
         const char* username = user.getName();
         if(!username || !*username)
+        {
+            DBGLOG("CLdapClient::updateUserPassword username must be provided");
             return false;
+        }
 
         if (currPassword)
         {
@@ -3208,7 +3201,10 @@ public:
     virtual bool updateUserPassword(const char* username, const char* newPassword)
     {
         if(!username || !*username)
+        {
+            DBGLOG("CLdapClient::updateUserPassword username must be provided");
             return false;
+        }
 
         const char* sysuser = m_ldapconfig->getSysUser();
         if(sysuser && *sysuser && strcmp(username, sysuser) == 0)
@@ -3539,7 +3535,10 @@ public:
     void addResourceTree(const char* name, const char* desc, IPropertyTree* elements)
     {
         if (!name || !*name)
+        {
+            DBGLOG("CLdapClient::addResourceTree resource name must be provided");
             return;
+        }
 
         Owned<IPTree> element = createPTree();
         element->addProp(getResourceFieldNames(RFName), name);
@@ -3689,7 +3688,10 @@ public:
     void addGroupTree(const char* name, const char* manageBy, const char* desc, IPropertyTree* groups)
     {
         if (!name || !*name)
+        {
+            DBGLOG("CLdapClient::addGroupTree groupname must be provided");
             return;
+        }
 
         Owned<IPTree> group = createPTree();
         group->addProp(getGroupFieldNames(GFName), name);
@@ -3861,7 +3863,10 @@ public:
             LDAPMessage *message;
 
             if(user == NULL || strlen(user) == 0)
+            {
+                DBGLOG("CLdapClient::getGroups username must be provided");
                 return;
+            }
             StringBuffer filter("sAMAccountName=");
             filter.append(user);
 
@@ -3952,10 +3957,16 @@ public:
     virtual bool deleteUser(ISecUser* user)
     {
         if(user == NULL)
+        {
+            DBGLOG("CLdapClient::deleteUser ISecUser must be provided");
             return false;
+        }
         const char* username = user->getName();
         if(username == NULL || *username == '\0')
+        {
+            DBGLOG("CLdapClient::deleteUser username must be provided");
             return false;
+        }
 
         StringBuffer userdn;
         getUserDN(username, userdn);
@@ -3999,7 +4010,10 @@ public:
     virtual void addGroup(const char* groupname, const char * groupOwner, const char * groupDesc, const char* basedn)
     {
         if(groupname == NULL || *groupname == '\0')
+        {
+            DBGLOG("CLdapClient::addGroup groupname must be provided");
             return;
+        }
 
         if(m_ldapconfig->getServerType() == ACTIVE_DIRECTORY)
         {
@@ -4378,7 +4392,11 @@ public:
     virtual bool isSuperUser(ISecUser* user)
     {
         if(user == NULL || user->getName() == NULL)
+        {
+            DBGLOG("CLdapClient::isSuperUser Populated ISecUser must be provided");
             return false;
+        }
+
         const char* username = user->getName();
         const char* sysuser = m_ldapconfig->getSysUser();
         if(sysuser != NULL && stricmp(sysuser, username) == 0)
@@ -4478,7 +4496,10 @@ private:
     virtual void addDC(const char* dc)
     {
         if(dc == NULL || *dc == '\0')
+        {
+            DBGLOG("CLdapClient::addDC dc must be provided");
             return;
+        }
 
         StringBuffer dcname;
         LdapUtils::getName(dc, dcname);
@@ -4599,7 +4620,10 @@ private:
     virtual void getUidFromDN(const char* dn, StringBuffer& uid)
     {
         if(dn == NULL || *dn == '\0')
+        {
+            DBGLOG("CLdapClient::getUidFromDN dn must be provided");
             return;
+        }
 
         if(m_ldapconfig->getServerType() != ACTIVE_DIRECTORY)
         {
@@ -4655,7 +4679,10 @@ private:
     virtual void getGroupDN(const char* groupname, StringBuffer& groupdn, const char * groupBaseDN=nullptr)
     {
         if(groupname == NULL)
+        {
+            DBGLOG("CLdapClient::getGroupDN groupname must be provided");
             return;
+        }
         LdapServerType stype = m_ldapconfig->getServerType();
         groupdn.append("cn=").append(groupname).append(",");
         if(stype == ACTIVE_DIRECTORY && stricmp(groupname, "Administrators") == 0)
@@ -4675,7 +4702,10 @@ private:
     virtual void getGroupBaseDN(const char* groupname, StringBuffer& groupbasedn, const char * groupBaseDN=nullptr)
     {
         if(groupname == NULL)
+        {
+            DBGLOG("CLdapClient::getGroupBaseDN groupname must be provided");
             return;
+        }
         LdapServerType stype = m_ldapconfig->getServerType();
         if(stype == ACTIVE_DIRECTORY && stricmp(groupname, "Administrators") == 0)
         {
@@ -4870,7 +4900,10 @@ private:
     {
         int len = sdlist.length();
         if(len == 0)
+        {
+            DBGLOG("CLdapClient::getSecurityDescriptors sdlist cannot be empty");
             return;
+        }
 
         const char* rbasedn = m_ldapconfig->getResourceBasedn(rtype);
         if(rbasedn == NULL || *rbasedn == '\0')
@@ -4931,7 +4964,10 @@ private:
 
         int len = sdlist.length();
         if(len == 0)
+        {
+            DBGLOG("CLdapClient::getSecurityDescriptors2 sdlist cannot be empty");
             return;
+        }
 
         StringBuffer filter;
         filter.append("(|");
@@ -5034,7 +5070,10 @@ private:
 
         int len = sdlist.length();
         if(len == 0)
+        {
+            DBGLOG("CLdapClient::getSecurityDescriptorsScope sdlist cannot be empty");
             return;
+        }
 
         LdapServerType servertype = m_ldapconfig->getServerType();
 
@@ -5206,11 +5245,17 @@ private:
     virtual void createLdapBasedn(ISecUser* user, const char* basedn, SecPermissionType ptype)
     {
         if(basedn == NULL || basedn[0] == '\0')
+        {
+            DBGLOG("CLdapClient::createLdapBasedn basedn must be provided");
             return;
+        }
 
         const char* ptr = strstr(basedn, "ou=");
         if(ptr == NULL)
+        {
+            DBGLOG("CLdapClient::createLdapBasedn OU= missing from basedn");
             return;
+        }
         ptr += 3;
 
         StringBuffer oubuf;
@@ -5236,10 +5281,16 @@ private:
     virtual bool addOrganizationalUnit(ISecUser* user, const char* name, const char* basedn, SecPermissionType ptype)
     {
         if(name == NULL || basedn == NULL)
+        {
+            DBGLOG("CLdapClient::addOrganizationalUnit OU name must be provided");
             return false;
+        }
 
         if(strchr(name, '/') != NULL || strchr(name, '=') != NULL)
+        {
+            DBGLOG("CLdapClient::addOrganizationalUnit Invalid characters in OU");
             return false;
+        }
 
         StringBuffer dn;
         dn.append("ou=").append(name).append(",").append(basedn);
@@ -5310,7 +5361,7 @@ private:
         {
             if(rc == LDAP_ALREADY_EXISTS)
             {
-                //WARNLOG("Can't insert ou=%s,%s to Ldap Server, an LDAP object with this name already exists", name, basedn);
+                WARNLOG("CLdapClient::addOrganizationalUnit LDAP object 'ou=%s,%s' already exists", name, basedn);
                 return false;
             }
             else
@@ -5358,7 +5409,10 @@ private:
     virtual void name2rdn(SecResourceType rtype, const char* resourcename, StringBuffer& ldapname)
     {
         if(resourcename == NULL || *resourcename == '\0')
+        {
+            DBGLOG("CLdapClient::name2rdn resourcename must be provided");
             return;
+        }
 
         if(m_ldapconfig->getServerType() == ACTIVE_DIRECTORY && (rtype == RT_DEFAULT || rtype == RT_MODULE || rtype == RT_SERVICE))
             ldapname.append("cn=");
@@ -5411,12 +5465,15 @@ private:
     virtual bool addResource(SecResourceType rtype, ISecUser& user, ISecResource* resource, SecPermissionType ptype, const char* basedn, CSecurityDescriptor* default_sd, bool lessException=true)
     {
         if(resource == NULL)
+        {
+            DBGLOG("CLdapClient::addResource can't add resource, ISecResource must be specified");
             return true;
+        }
 
         char* resourcename = (char*)resource->getName();
         if(resourcename == NULL)
         {
-            DBGLOG("can't add resource, empty resource name");
+            DBGLOG("CLdapClient::addResource can't add resource, empty resource name");
             return false;
         }
 
@@ -5431,12 +5488,15 @@ private:
         }
         if(rbasedn == NULL || *rbasedn == '\0')
         {
-            DBGLOG("Can't add resource, corresponding resource basedn is not defined");
+            DBGLOG("CLdapClient::addResource Can't add resource '%s', corresponding resource basedn is not defined",resourcename);
             return false;
         }
 
         if(strchr(resourcename, '/') != NULL || strchr(resourcename, '=') != NULL)
+        {
+            DBGLOG("CLdapClient::addResource Can't add resource '%s', invalid characters specified",resourcename);
             return false;
+        }
 
         if(rtype == RT_FILE_SCOPE || rtype == RT_WORKUNIT_SCOPE)
         {
@@ -5593,7 +5653,10 @@ private:
             {
                 //WARNLOG("Can't insert %s to Ldap Server, an LDAP object with this name already exists", resourcename);
                 if(lessException)
+                {
+                    DBGLOG("CLdapClient::addResource Can't add resource '%s', an LDAP object with this name already exists", resourcename);
                     return false;
+                }
                 else
                     throw MakeStringException(-1, "Can't insert %s, an LDAP object with this name already exists", resourcename);
             }

system/security/LdapSecurity/ldapsecurity.cpp

@@ -419,7 +419,10 @@ bool CLdapSecResourceList::copyTo(ISecResourceList& destination)
 ISecResource* CLdapSecResourceList::addResource(const char * name)
 {
     if(!name || !*name)
+    {
+        DBGLOG("CLdapSecResourceList::addResource resource name must be provided");
         return NULL;
+    }
 
     ISecResource* resource = m_rmap[name];
     if(resource == NULL)
@@ -435,10 +438,16 @@ ISecResource* CLdapSecResourceList::addResource(const char * name)
 void CLdapSecResourceList::addResource(ISecResource * resource)
 {
     if(resource == NULL)
+    {
+        DBGLOG("CLdapSecResourceList::addResource2 ISecResource cannot be NULL");
         return;
+    }
     const char* name = resource->getName();
     if(!name || !*name)
+    {
+        DBGLOG("CLdapSecResourceList::addResource2 resource name must be provided");
         return;
+    }
 
     ISecResource* r = m_rmap[name];
     if(r == NULL)
@@ -456,7 +465,10 @@ bool CLdapSecResourceList::addCustomResource(const char * name, const char * con
 ISecResource * CLdapSecResourceList::getResource(const char * Resource)
 {
     if(!Resource || !*Resource)
+    {
+        DBGLOG("CLdapSecResourceList::getResource resource name must be provided");
         return NULL;
+    }
 
     ISecResource* r = m_rmap[Resource];
     if(r)
@@ -496,7 +508,10 @@ ISecPropertyIterator * CLdapSecResourceList::getPropertyItr()
 ISecProperty* CLdapSecResourceList::findProperty(const char* name)
 {
     if(!name || !*name)
+    {
+        DBGLOG("CLdapSecResourceList::findProperty property name must be provided");
         return NULL;
+    }
     return m_rmap[name];
 }
 
@@ -604,7 +619,10 @@ bool CLdapSecManager::unsubscribe(ISecAuthenticEvents & events)
 bool CLdapSecManager::authenticate(ISecUser* user)
 {
     if(!user)
+    {
+        DBGLOG("CLdapSecManager::authenticate user cannot be NULL");
         return false;
+    }
 
     if(user->getAuthenticateStatus() == AS_AUTHENTICATED)
         return true;
@@ -1068,7 +1086,10 @@ ISecItemIterator* CLdapSecManager::getResourcesSorted(SecResourceType rtype, con
 void CLdapSecManager::setExtraParam(const char * name, const char * value)
 {
     if(name == NULL || name[0] == '\0')
+    {
+        DBGLOG("CLdapSecManager::setExtraParam name must be provided");
         return;
+    }
 
     if (!m_extraparams)
         m_extraparams.setown(createProperties(false));
@@ -1335,7 +1356,7 @@ bool CLdapSecManager::createUserScopes()
         ISecUser &user = it->get();
         if (!m_ldap_client->createUserScope(user))
         {
-            PROGLOG("Error creating scope for user '%s'", user.getName());
+            PROGLOG("CLdapSecManager::createUserScopes Error creating user scope for user '%s'", user.getName());
             rc = false;
         }
         it->next();