Review comments

Introduce a persistent decorator instance to the HTTP binding, ESP context, and
security handler to avoid repeated construction and destruction.

Improve the binding's handling of security manager creation failures to avoid
null dereferences when creating the decorator, or resource auth maps.

esp/bindings/bind_ng.hpp

@@ -22,6 +22,7 @@
 #include "jliball.hpp"
 #include "esp.hpp"
 #include "soapesp.hpp"
+#include "secmanagertracedecorator.hpp"
 
 class CEspNgContext  : implements IEspContext, public CInterface
 {
@@ -31,6 +32,7 @@ class CEspNgContext  : implements IEspContext, public CInterface
 
     Owned<ISecUser> secuser;
     Owned<ISecManager> secmgr;
+    Owned<ISecManager> tracingSecMgrDecorator;
     Owned<ISecResourceList> reslist;
     Owned<IAuthMap> authMap;
     Owned<ISecPropertyList> secprops;
@@ -78,7 +80,20 @@ class CEspNgContext  : implements IEspContext, public CInterface
     }
 
 
-    void setSecManger(ISecManager * mgr){secmgr.setown(mgr);}
+    void setSecManger(ISecManager * mgr)
+    {
+        setSecManager(mgr, nullptr);
+    }
+    virtual void setSecManager(ISecManager* mgr, ISecManager * _tracingSecMgrDecorator)
+    {
+        secmgr.setown(mgr);
+        if (!mgr)
+            tracingSecMgrDecorator.clear();
+        else if (!_tracingSecMgrDecorator)
+            tracingSecMgrDecorator.setown(new CSecManagerTraceDecorator(*mgr));
+        else
+            tracingSecMgrDecorator.set(_tracingSecMgrDecorator);
+    }
     ISecManager * querySecManager(){return secmgr.get();}
 
     void setContextPath(const char * path){contextPath.clear().append(path);}

esp/bindings/http/platform/httpbinding.cpp

@@ -406,6 +406,9 @@ EspHttpBinding::EspHttpBinding(IPropertyTree* tree, const char *bindname, const
                     //This is a Pluggable Security Manager
                     setBndCfgServiceType(tree, procname, bnd_cfg);
                     m_secmgr.setown(SecLoader::loadPluggableSecManager<ISecManager>(bindname, bnd_cfg, secMgrCfg));
+                    if (!m_secmgr)
+                        throw makeStringException(-1, "error loading pluggable security manager");
+                    m_tracingSecMgrDecorator.setown(new CSecManagerTraceDecorator(*m_secmgr));
                     m_authmap.setown(m_secmgr->createAuthMap(authcfg));
                     m_feature_authmap.setown(m_secmgr->createFeatureMap(authcfg));
                     m_setting_authmap.setown(m_secmgr->createSettingMap(authcfg));
@@ -434,6 +437,7 @@ EspHttpBinding::EspHttpBinding(IPropertyTree* tree, const char *bindname, const
                         {
                             throw MakeStringException(-1, "error generating SecManager");
                         }
+                        m_tracingSecMgrDecorator.setown(new CSecManagerTraceDecorator(*m_secmgr));;
 
                         StringBuffer basednbuf;
                         authcfg->getProp("@resourcesBasedn", basednbuf);
@@ -449,6 +453,9 @@ EspHttpBinding::EspHttpBinding(IPropertyTree* tree, const char *bindname, const
                     else if(stricmp(m_authmethod.str(), "Local") == 0)
                     {
                         m_secmgr.setown(SecLoader::loadSecManager("Local", "EspHttpBinding", NULL));
+                        if (m_secmgr.get() == NULL)
+                            throw MakeStringException(-1, "error loading local security manager");
+                        m_tracingSecMgrDecorator.setown(new CSecManagerTraceDecorator(*m_secmgr));
                         m_authmap.setown(m_secmgr->createAuthMap(authcfg));
                     }
                     IRestartManager* restartManager = dynamic_cast<IRestartManager*>(m_secmgr.get());
@@ -847,7 +854,7 @@ void EspHttpBinding::populateRequest(CHttpRequest *request)
 {
     IEspContext* ctx = request->queryContext();
 
-    ctx->setSecManger(m_secmgr.getLink());
+    ctx->setSecManager(m_secmgr.getLink(), m_tracingSecMgrDecorator.get());
     ctx->setFeatureAuthMap(m_feature_authmap.getLink());
 
     StringBuffer userid, password,realm,peer;
@@ -972,7 +979,7 @@ bool EspHttpBinding::basicAuth(IEspContext* ctx)
         return false;
     }
 
-    bool authenticated = CSecManagerTraceDecorator(*m_secmgr).authorize(*user, rlist, ctx->querySecureContext());
+    bool authenticated = m_tracingSecMgrDecorator->authorize(*user, rlist, ctx->querySecureContext());
     if(!authenticated)
     {
         VStringBuffer err("User %s : ", user->getName());
@@ -1029,7 +1036,7 @@ bool EspHttpBinding::basicAuth(IEspContext* ctx)
     if(securitySettings == NULL)
         return authorized;
 
-    CSecManagerTraceDecorator(*m_secmgr).updateSettings(*user,securitySettings, ctx->querySecureContext());
+    m_tracingSecMgrDecorator->updateSettings(*user,securitySettings, ctx->querySecureContext());
 
     ctx->addTraceSummaryTimeStamp(LogMin, "basicAuth", TXSUMMARY_GRP_CORE);
     return authorized;

esp/bindings/http/platform/httpbinding.hpp

@@ -153,6 +153,7 @@ class esp_http_decl EspHttpBinding :
     StringBuffer            m_filespath;
     StringBuffer            m_wsdlAddress;
     Owned<ISecManager>      m_secmgr;
+    Owned<ISecManager>      m_tracingSecMgrDecorator;
     Owned<IAuthMap>         m_authmap;
     Owned<IAuthMap>         m_feature_authmap;
     Owned<IAuthMap>         m_setting_authmap;
@@ -372,6 +373,7 @@ class esp_http_decl EspHttpBinding :
         return false;
     }
     ISecManager* querySecManager() const { return m_secmgr.get(); }
+    ISecManager* queryTracingSecManager() const { return m_tracingSecMgrDecorator.get(); }
     IAuthMap* queryAuthMAP() const { return m_authmap.get();}
     const char* queryAuthMethod() const { return m_authmethod.str(); }
     void setProcessName(const char* name) { processName.set(name); }

esp/platform/esp.hpp

@@ -125,7 +125,9 @@ interface IEspContext : extends IInterface
     virtual void setResources(ISecResourceList * rlist) = 0;
     virtual ISecResourceList * queryResources() = 0;
     virtual void setSecManger(ISecManager * mgr) = 0;
+    virtual void setSecManager(ISecManager * mgr, ISecManager * tracingSecMgrDecoratorDecorator) = 0;
     virtual ISecManager * querySecManager() = 0;
+    virtual ISecManager * queryTracingSecManager() = 0;
     virtual void setContextPath(const char * path) = 0;
     virtual const char * getContextPath() = 0;
     virtual void setBindingValue(void * value) = 0;

esp/platform/espcontext.cpp

@@ -28,6 +28,7 @@
 #include "espsecurecontext.hpp"
 #include "ldapsecurity.ipp"
 #include "dasds.hpp"
+#include "secmanagertracedecorator.hpp"
 
 class CEspContext : public CInterface, implements IEspContext
 {
@@ -50,6 +51,7 @@ class CEspContext : public CInterface, implements IEspContext
     Owned<ISecUser> m_user;
     Owned<ISecResourceList> m_resources;
     Owned<ISecManager> m_secmgr;
+    Owned<ISecManager> m_tracingSecMgrDecorator;
     Owned<IAuthMap> m_feature_authmap;
     Owned<ISecPropertyList> m_sec_settings;
 
@@ -289,16 +291,32 @@ class CEspContext : public CInterface, implements IEspContext
     }
 
     virtual void setSecManger(ISecManager* mgr)
+    {
+        setSecManager(mgr, nullptr);
+    }
+
+    virtual void setSecManager(ISecManager* mgr, ISecManager* tracingSecMgrDecorator) override
     {
         m_secmgr.setown(mgr);
-        m_SecurityHandler.setSecManger(mgr);
+        if (!mgr)
+            m_tracingSecMgrDecorator.clear();
+        else if (!tracingSecMgrDecorator)
+            m_tracingSecMgrDecorator.setown(new CSecManagerTraceDecorator(*mgr));
+        else
+            m_tracingSecMgrDecorator.set(tracingSecMgrDecorator);
+        m_SecurityHandler.setSecManager(mgr, tracingSecMgrDecorator);
     }
 
     virtual ISecManager* querySecManager()
     {
         return m_secmgr.get();
     }
 
+    virtual ISecManager* queryTracingSecManager() override
+    {
+        return m_tracingSecMgrDecorator.get();
+    }
+
     virtual void setBindingValue(void * value)
     {
         m_bindingValue=value;

esp/platform/sechandler.cpp

@@ -41,8 +41,19 @@ SecHandler::~SecHandler()
 }
 
 void SecHandler::setSecManger(ISecManager* mgr)
+{
+    setSecManager(mgr, nullptr);
+}
+
+void SecHandler::setSecManager(ISecManager* mgr, ISecManager* tracingSecMgrDecoratorDecorator)
 {
     m_secmgr.set(mgr);
+    if (!mgr)
+        m_tracingSecMgrDecorator.clear();
+    else if (!tracingSecMgrDecoratorDecorator)
+        m_tracingSecMgrDecorator.setown(new CSecManagerTraceDecorator(*mgr));
+    else
+        m_tracingSecMgrDecorator.set(tracingSecMgrDecoratorDecorator);
 }
 
 
@@ -136,7 +147,7 @@ bool SecHandler::authorizeSecFeature(const char * pszFeatureUrl, const char* Use
 
 bool SecHandler::authorizeTrial(ISecUser& user,const char* pszFeatureUrl, SecAccessFlags & required_access)
 {
-    int trial_access = CSecManagerTraceDecorator(*m_secmgr).authorizeEx(RT_TRIAL,user,pszFeatureUrl);
+    int trial_access = m_tracingSecMgrDecorator->authorizeEx(RT_TRIAL,user,pszFeatureUrl);
     if(trial_access < required_access)
         throw MakeStringException(201,"Your company has used up all of their free transaction credits");
     return true;
@@ -267,7 +278,7 @@ bool SecHandler::authorizeSecReqFeatures(StringArray & features, IEspStringIntMa
     bool auth_ok = false;
     try
     {
-        auth_ok = CSecManagerTraceDecorator(*m_secmgr).authorize(*m_user.get(), plist, m_secureContext.get());
+        auth_ok = m_tracingSecMgrDecorator->authorize(*m_user.get(), plist, m_secureContext.get());
     }
     catch(IException* e)
     {

esp/platform/sechandler.hpp

@@ -28,6 +28,7 @@
 class SecHandler : public CInterface
 {
     Owned<ISecManager> m_secmgr;
+    Owned<ISecManager> m_tracingSecMgrDecorator;
     Owned<ISecResourceList> m_resources;
     Owned<ISecUser> m_user;
     Owned<IAuthMap> m_feature_authmap;
@@ -49,6 +50,7 @@ class SecHandler : public CInterface
     bool authorizeSecFeature(const char * pszFeatureUrl, const char* UserID, const char* CompanyID, SecAccessFlags & access,bool bCheckTrial,int DebitUnits, SecUserStatus & user_status);
 
     void setSecManger(ISecManager* mgr);
+    void setSecManager(ISecManager* mgr, ISecManager* tracingSecMgrDecoratorDecorator);
     void setResources(ISecResourceList* rlist);
     void setUser(ISecUser* user);
     void setFeatureAuthMap(IAuthMap * map);

esp/services/ws_dfu/ws_dfuService.cpp

@@ -2021,7 +2021,7 @@ static void getFilePermission(CDfsLogicalFileName &dlfn, ISecUser & user, IUserD
             StringBuffer scopes;
             dlfn.getScopes(scopes);
 
-            permissionTemp = CSecManagerTraceDecorator(*secmgr).authorizeFileScope(user, scopes.str());
+            permissionTemp = secmgr->authorizeFileScope(user, scopes.str());
         }
 
         //Descrease the permission whenever a component has a lower permission.
@@ -2034,7 +2034,7 @@ static void getFilePermission(CDfsLogicalFileName &dlfn, ISecUser & user, IUserD
 
 bool CWsDfuEx::getUserFilePermission(IEspContext &context, IUserDescriptor* udesc, const char* logicalName, SecAccessFlags& permission)
 {
-    ISecManager* secmgr = context.querySecManager();
+    ISecManager* secmgr = context.queryTracingSecManager();
     if (!secmgr)
     {
         return false;

helm/examples/esp/README.md

@@ -47,6 +47,7 @@ esp:
 - name: eclwatch
   traceFlags:
     traceLevel: 2
+    traceSecMgr: true
 ```
 
 ## Cluster Overrides
@@ -61,7 +62,7 @@ The previous YAML example may be reproduced in XML with the following:
 
 ```xml
 <EspProcess ...>
-  <traceFlags traceLevel="2" />
+  <traceFlags traceLevel="2" traceSecMgr="1"/>
   ...
 <EspProcess>
 ```

system/security/shared/secmanagertracedecorator.hpp

@@ -22,54 +22,11 @@
 #include "jthread.hpp"
 #include "jtrace.hpp"
 
-/**
- * @brief Templated utility class to decorate an object with additional functionality.
- *
- * A decorator implements the same named interface as the objects it decorates. This class extends
- * the interface, leaving implementation to subclasses. This class requires the decorated object to
- * implement a named interface, even if only IInterface.
- *
- * In the ideal situation, the decorated object's interface is described completely by a named
- * interface. By implementing the same interface, a decorator is interchangeable with the object
- * it decoratos.
- *
- * In less than ideal situations, the decorated object's interface is an extension of a named
- * interface. The decorator extends the extends the named interface, with subclasses required to
- * implement both the named interface and all extensions. The decorator should then be
- * interchangeable with its decorated object as a templated argument, but not be cast to the
- * decorated type.
- *
- * The less ideal scenario is suported by two template parameters. The ideal situation requires
- * only the first.
- * - decorated_t is the type of the object to be decorated. If the the decorated object conforms
- *   to an interface, use of the interface is preferred.
- * - secorated_interface_t is the interface implemented by the decorated object. If not the same
- *   as decorated_t, it is assumed to be a base of that type.
- *
- * Consider the example of ISecManager, generally, and the special case of CLdapSecManager. For
- * most security managers, both template parameters may be ISecManager. CLdapSecManager is an
- * exception because it exposes additional interfaces not included in ISecManager or any other
- * interface. In this case, decorated_t should be CLdapSecManager and decorated_interface_t should
- * be ISecManager.
- */
-template <typename decorated_t, typename decorated_interface_t = decorated_t>
-class TDecorator : public CInterfaceOf<decorated_interface_t>
-{
-protected:
-    Linked<decorated_t> decorated;
-
-public:
-    TDecorator(decorated_t &_decorated) : decorated(&_decorated) {}
-    virtual ~TDecorator() {}
-    decorated_t *queryDecorated() { return decorated.get(); }
-    decorated_t *getDecorated() { return decorated.getLink(); }
-};
-
 /**
  * @brief Macro used start tracing a block of code in the security manager decorator.
  *
  * Create a new named internal span and enter a try block. Used with END_SEC_MANAGER_TRACE_BLOCK,
- * provides consistent timing and exception handling for the inned code block.
+ * provides consistent timing and exception handling for the inner code block.
  *
  * Some security manager requests include outgoing remote calls, but the ESP does not assume
  * which requests are remote. Managers making remote calls may considier creating client spans
@@ -122,11 +79,9 @@ class TDecorator : public CInterfaceOf<decorated_interface_t>
  * - If nothing else changes, create a subclasses of TSecManagerTraceDecorator, with template
  *   parameters CLdapSecManager and ISecManager, to decorate the LDAP-specific interfaces.
  */
-template <typename secmgr_t = ISecManager, typename secmgr_interface_t = ISecManager>
-class TSecManagerTraceDecorator : public TDecorator<secmgr_t, secmgr_interface_t>
+template <typename secmgr_t = ISecManager>
+class TSecManagerTraceDecorator : public CInterfaceOf<ISecManager>
 {
-    using TDecorator<secmgr_t, secmgr_interface_t>::decorated;
-
 public:
     virtual SecFeatureSet queryFeatures(SecFeatureSupportLevel level) const override
     {
@@ -359,17 +314,12 @@ class TSecManagerTraceDecorator : public TDecorator<secmgr_t, secmgr_interface_t
     }
 
 protected:
-    SecFeatureSet implemented = 0; /// The decorated manager instance's implemented feature set.
-    bool tracing = true; /// Flag indicating if tracing is enabled.
+    Linked<secmgr_t> decorated;
+    bool tracing = queryTraceManager().isTracingEnabled();
 
 public:
     TSecManagerTraceDecorator(secmgr_t &_decorated)
-        : TDecorator<secmgr_t, secmgr_interface_t>(_decorated)
-        , implemented(_decorated.queryFeatures(SecFeatureSupportLevel::SFSL_Implemented))
-        , tracing(queryTraceManager().isTracingEnabled() && doTrace(traceSecMgr))
-    {
-    }
-    virtual ~TSecManagerTraceDecorator()
+        : decorated(&_decorated)
     {
     }
 
@@ -684,7 +634,7 @@ class TSecManagerTraceDecorator : public TDecorator<secmgr_t, secmgr_interface_t
      */
     inline bool doTraceFeature(SecFeatureBit feature) const
     {
-        return (tracing && (implemented & feature) != 0);
+        return (tracing && doTrace(traceSecMgr) && decorated->checkImplementedFeatures(feature));
     }
 };