Merge pull request #9744 from RussWhitehead/logLDAP640

HPCC-17279 Improve LDAP logging

Reviewed-By: Kevin Wang <kevin.wang@lexisnexis.com>
Reviewed-By: Richard Chapman <rchapman@hpccsystems.com>

system/security/LdapSecurity/ldapconnection.cpp

@@ -1475,7 +1475,10 @@ 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;
@@ -1719,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))
@@ -2224,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;
             }
         }
@@ -2523,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;
@@ -2625,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);
@@ -3168,7 +3180,10 @@ public:
     {
         const char* username = user.getName();
         if(!username || !*username)
+        {
+            DBGLOG("CLdapClient::updateUserPassword username must be provided");
             return false;
+        }
 
         if (currPassword)
         {
@@ -3186,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)
@@ -3517,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);
@@ -3667,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);
@@ -3839,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);
 
@@ -3930,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);
@@ -3977,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)
         {
@@ -4356,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)
@@ -4456,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);
@@ -4577,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)
         {
@@ -4633,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)
@@ -4653,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)
         {
@@ -4848,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')
@@ -4909,7 +4964,10 @@ private:
 
         int len = sdlist.length();
         if(len == 0)
+        {
+            DBGLOG("CLdapClient::getSecurityDescriptors2 sdlist cannot be empty");
             return;
+        }
 
         StringBuffer filter;
         filter.append("(|");
@@ -5012,7 +5070,10 @@ private:
 
         int len = sdlist.length();
         if(len == 0)
+        {
+            DBGLOG("CLdapClient::getSecurityDescriptorsScope sdlist cannot be empty");
             return;
+        }
 
         LdapServerType servertype = m_ldapconfig->getServerType();
 
@@ -5184,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;
@@ -5214,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);
@@ -5288,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
@@ -5336,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=");
@@ -5389,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;
         }
 
@@ -5409,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)
         {
@@ -5571,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();