[cpp-sp] branch main updated: Extending error handling and exception logging.
Scott Cantor
cantor.2 at osu.edu
Tue Feb 11 16:50:47 UTC 2025
This is an automated email from the git hooks/post-receive script.
scantor pushed a commit to branch main
in repository cpp-sp.
View the commit online:
http://git.shibboleth.net/view/?p=cpp-sp.git;a=commit;h=21cfa21333fd9fcb24bca10f11dcaaf5216389e6
The following commit(s) were added to refs/heads/main by this push:
new 21cfa213 Extending error handling and exception logging.
21cfa213 is described below
commit 21cfa21333fd9fcb24bca10f11dcaaf5216389e6
Author: Scott Cantor <cantor.2 at osu.edu>
AuthorDate: Tue Feb 11 11:50:35 2025 -0500
Extending error handling and exception logging.
---
configs/shibboleth.ini | 2 +-
shibsp/AbstractSPRequest.cpp | 2 +-
shibsp/Agent.cpp | 47 +++++++++++++---------
shibsp/Agent.h | 2 +-
shibsp/exceptions.cpp | 45 +++++++++++++++------
shibsp/exceptions.h | 27 ++++++++-----
.../handler/impl/DefaultHandlerConfiguration.cpp | 8 +++-
shibsp/handler/impl/SessionInitiator.cpp | 21 ++++++----
shibsp/remoting/impl/AbstractRemotingService.cpp | 5 ++-
9 files changed, 106 insertions(+), 53 deletions(-)
diff --git a/configs/shibboleth.ini b/configs/shibboleth.ini
index b015982a..bbca9770 100644
--- a/configs/shibboleth.ini
+++ b/configs/shibboleth.ini
@@ -11,7 +11,7 @@
[remoting]
-baseURL = https://localhost/idp/profile/sp
+baseURL = https://localhost/idp/profile/sp/
agentID = sp.example.org
authMethod = basic
authCachingCookie = __Host-JSESSIONID
diff --git a/shibsp/AbstractSPRequest.cpp b/shibsp/AbstractSPRequest.cpp
index 8abdf5ff..30e0e97f 100644
--- a/shibsp/AbstractSPRequest.cpp
+++ b/shibsp/AbstractSPRequest.cpp
@@ -453,7 +453,7 @@ void AbstractSPRequest::limitRedirect(const char* url) const
}
m_log.warn("redirectLimit policy enforced, blocked redirect to (%s)", url);
- throw agent_exception("Blocked unacceptable redirect location.");
+ throw AgentException("Blocked unacceptable redirect location.");
}
}
diff --git a/shibsp/Agent.cpp b/shibsp/Agent.cpp
index 9436c0ab..079fc08f 100644
--- a/shibsp/Agent.cpp
+++ b/shibsp/Agent.cpp
@@ -67,13 +67,27 @@ Agent::~Agent()
{
}
-long Agent::handleError(Category& log, SPRequest& request, const Session* session, const exception* ex, bool mayRedirect) const
+long Agent::handleError(Category& log, SPRequest& request, const Session* session, exception* ex, bool mayRedirect) const
{
// The properties we need can be set in the RequestMap, or the Errors element.
bool externalParameters = false;
const char* redirectErrors = nullptr;
- const agent_exception* richEx = dynamic_cast<const agent_exception*>(ex);
+ AgentException* richEx = dynamic_cast<AgentException*>(ex);
+
+ if (ex) {
+ // Populate target if needed.
+ if (!richEx->getProperty("target")) {
+ richEx->addProperty("target", request.getRequestURL());
+ }
+
+ if (richEx) {
+ richEx->log(request);
+ }
+ else {
+ request.log(Priority::SHIB_ERROR, ex->what());
+ }
+ }
// Now look for settings in the request map.
try {
@@ -82,24 +96,25 @@ long Agent::handleError(Category& log, SPRequest& request, const Session* sessio
if (mayRedirect)
redirectErrors = settings.first->getString("redirectErrors");
}
- catch (const exception& ex) {
- log.error(ex.what());
+ catch (const exception& nested) {
+ request.log(Priority::SHIB_ERROR, nested.what());
}
// Check for redirection on errors.
if (mayRedirect && redirectErrors) {
string loc(redirectErrors);
request.absolutize(loc);
- const agent_exception* richEx = dynamic_cast<const agent_exception*>(ex);
if (richEx) {
- // TODO: probably alter how this works or what's included.
+ // TODO: alter how this works or what's included.
loc = loc + '?' + richEx->toQueryString();
}
return request.sendRedirect(loc.c_str());
}
- // TODO: this probably changes significantly, but ultimately we're trying to pass
- // back a status code.
+ // TODO: this probably changes significantly. The status code isn't all that material,
+ // but we could potentially use a custom code to facilitate custom error pages.
+ // The big addition would be exporting exception propertties into the request
+ // so Apache can surface them using its error redirection feature.
istringstream msg("Internal Server Error. Please contact the site administrator.");
return request.sendResponse(msg, richEx ? richEx->getStatusCode() : HTTPResponse::SHIBSP_HTTP_STATUS_ERROR);
@@ -255,7 +270,7 @@ pair<bool,long> Agent::doAuthentication(SPRequest& request, bool handler) const
return make_pair(true, request.sendRedirect(redirectURL.c_str()));
}
else {
- agent_exception ex("Access via unencrypted HTTP was blocked.");
+ AgentException ex("Access via unencrypted HTTP was blocked.");
return make_pair(true, handleError(log, request, nullptr, &ex, false));
}
}
@@ -361,8 +376,7 @@ pair<bool,long> Agent::doAuthentication(SPRequest& request, bool handler) const
log.debug("doAuthentication succeeded");
return make_pair(false,0L);
}
- catch (const exception& e) {
- request.log(Priority::SHIB_ERROR, e.what());
+ catch (exception& e) {
return make_pair(true, handleError(log, request, nullptr, &e));
}
}
@@ -414,7 +428,7 @@ pair<bool,long> Agent::doAuthorization(SPRequest& request) const
case AccessControl::shib_acl_false:
{
log.warn("access control provider denied access");
- agent_exception ex("Access to resource denied.");
+ AgentException ex("Access to resource denied.");
ex.setStatusCode(HTTPResponse::SHIBSP_HTTP_STATUS_FORBIDDEN);
return make_pair(true, handleError(log, request, session, &ex, false));
}
@@ -428,8 +442,7 @@ pair<bool,long> Agent::doAuthorization(SPRequest& request) const
return make_pair(true, request.returnDecline());
}
}
- catch (const exception& e) {
- request.log(Priority::SHIB_ERROR, e.what());
+ catch (exception& e) {
return make_pair(true, handleError(log, request, nullptr, &e));
}
}
@@ -512,8 +525,7 @@ pair<bool,long> Agent::doExport(SPRequest& request, bool requireSession) const
return make_pair(false,0L);
}
- catch (const exception& e) {
- request.log(Priority::SHIB_ERROR, e.what());
+ catch (exception& e) {
return make_pair(true, handleError(log, request, session, &e));
}
}
@@ -573,8 +585,7 @@ pair<bool,long> Agent::doHandler(SPRequest& request) const
}
throw ConfigurationException("Configured Shibboleth handler failed to process the request.");
}
- catch (const exception& e) {
- request.log(Priority::SHIB_ERROR, e.what());
+ catch (exception& e) {
Session* session = nullptr;
try {
session = request.getSession(false, true, false); // do not cache
diff --git a/shibsp/Agent.h b/shibsp/Agent.h
index 56cac450..d0294d37 100644
--- a/shibsp/Agent.h
+++ b/shibsp/Agent.h
@@ -172,7 +172,7 @@ namespace shibsp {
Category& log,
SPRequest& request,
const Session* session=nullptr,
- const std::exception* ex=nullptr,
+ std::exception* ex=nullptr,
bool mayRedirect=true
) const;
void clearHeaders(SPRequest& request) const;
diff --git a/shibsp/exceptions.cpp b/shibsp/exceptions.cpp
index b3cbd448..80cd0510 100644
--- a/shibsp/exceptions.cpp
+++ b/shibsp/exceptions.cpp
@@ -19,69 +19,73 @@
*/
#include "internal.h"
-#include "AgentConfig.h"
#include "exceptions.h"
+#include "AgentConfig.h"
+#include "SPRequest.h"
#include "io/HTTPResponse.h"
+#include "logging/Priority.h"
#include "util/URLEncoder.h"
+#include <sstream>
+
using namespace shibsp;
using namespace std;
-agent_exception::agent_exception(const char* msg) : m_status(HTTPResponse::SHIBSP_HTTP_STATUS_ERROR)
+AgentException::AgentException(const char* msg) : m_status(HTTPResponse::SHIBSP_HTTP_STATUS_ERROR)
{
if (msg)
m_msg = msg;
}
-agent_exception::agent_exception(const string& msg) : m_status(HTTPResponse::SHIBSP_HTTP_STATUS_ERROR), m_msg(msg)
+AgentException::AgentException(const string& msg) : m_status(HTTPResponse::SHIBSP_HTTP_STATUS_ERROR), m_msg(msg)
{
}
-agent_exception::~agent_exception() noexcept
+AgentException::~AgentException() noexcept
{
}
-const char* agent_exception::what() const noexcept
+const char* AgentException::what() const noexcept
{
return m_msg.c_str();
}
-int agent_exception::getStatusCode() const noexcept
+int AgentException::getStatusCode() const noexcept
{
return m_status;
}
-void agent_exception::setStatusCode(int code) noexcept
+void AgentException::setStatusCode(int code) noexcept
{
m_status = code;
}
-const char* agent_exception::getProperty(const char* name) const noexcept
+const char* AgentException::getProperty(const char* name) const noexcept
{
const auto& prop = m_props.find(name);
return prop != m_props.end() ? prop->second.c_str() : nullptr;
}
-const unordered_map<string,string>& agent_exception::getProperties() const noexcept
+const unordered_map<string,string>& AgentException::getProperties() const noexcept
{
return m_props;
}
-void agent_exception::addProperties(const unordered_map<string,string>& props)
+void AgentException::addProperties(const unordered_map<string,string>& props)
{
for (const auto& p : props) {
m_props.insert(p);
}
}
-void agent_exception::addProperty(const char* name, const char* value)
+void AgentException::addProperty(const char* name, const char* value)
{
if (name && value) {
m_props[name] = value;
}
}
-string agent_exception::toQueryString() const
+string AgentException::toQueryString() const
{
string q;
const URLEncoder& enc = AgentConfig::getConfig().getURLEncoder();
@@ -92,3 +96,20 @@ string agent_exception::toQueryString() const
}
return q;
}
+
+void AgentException::log(const SPRequest& request) const
+{
+ ostringstream msg;
+ msg << what() << ": [";
+
+ // Dump properties and status code.
+ msg << "status=" << getStatusCode();
+
+ for (const auto& prop : m_props) {
+ msg << ", " << prop.first << '=' << prop.second;
+ }
+
+ msg << ']';
+
+ request.log(Priority::SHIB_ERROR, msg.str());
+}
diff --git a/shibsp/exceptions.h b/shibsp/exceptions.h
index fc80f47a..809bcbc8 100644
--- a/shibsp/exceptions.h
+++ b/shibsp/exceptions.h
@@ -49,27 +49,29 @@ namespace shibsp {
#pragma warning( disable : 4250 4251 )
#endif
+ class SHIBSP_API SPRequest;
+
/**
* Base exception class, supports attaching additional data for error handling.
*/
- class SHIBSP_EXCEPTIONAPI(SHIBSP_API) agent_exception : public std::exception
+ class SHIBSP_EXCEPTIONAPI(SHIBSP_API) AgentException : public std::exception
{
public:
- virtual ~agent_exception() noexcept;
+ virtual ~AgentException() noexcept;
/**
* Constructs an exception using a message.
*
* @param msg error message
*/
- agent_exception(const char* msg=nullptr);
+ AgentException(const char* msg=nullptr);
/**
* Constructs an exception using a message.
*
* @param msg error message
*/
- agent_exception(const std::string& msg);
+ AgentException(const std::string& msg);
/**
* Returns the error message, after processing any parameter references.
@@ -131,18 +133,25 @@ namespace shibsp {
*/
std::string toQueryString() const;
+ /**
+ * Log an error through this request using the exception properties as input.
+ *
+ * @param request SP request
+ */
+ void log(const SPRequest& request) const;
+
private:
int m_status;
std::string m_msg;
std::unordered_map<std::string,std::string> m_props;
};
- DECL_SHIBSP_EXCEPTION(AttributeException,SHIBSP_EXCEPTIONAPI(SHIBSP_API),shibsp::agent_exception);
- DECL_SHIBSP_EXCEPTION(ConfigurationException,SHIBSP_EXCEPTIONAPI(SHIBSP_API),shibsp::agent_exception);
- DECL_SHIBSP_EXCEPTION(IOException,SHIBSP_EXCEPTIONAPI(SHIBSP_API),shibsp::agent_exception);
- DECL_SHIBSP_EXCEPTION(RemotingException,SHIBSP_EXCEPTIONAPI(SHIBSP_API),shibsp::agent_exception);
+ DECL_SHIBSP_EXCEPTION(AttributeException,SHIBSP_EXCEPTIONAPI(SHIBSP_API),shibsp::AgentException);
+ DECL_SHIBSP_EXCEPTION(ConfigurationException,SHIBSP_EXCEPTIONAPI(SHIBSP_API),shibsp::AgentException);
+ DECL_SHIBSP_EXCEPTION(IOException,SHIBSP_EXCEPTIONAPI(SHIBSP_API),shibsp::AgentException);
+ DECL_SHIBSP_EXCEPTION(RemotingException,SHIBSP_EXCEPTIONAPI(SHIBSP_API),shibsp::AgentException);
DECL_SHIBSP_EXCEPTION(OperationException,SHIBSP_EXCEPTIONAPI(SHIBSP_API),shibsp::RemotingException);
- DECL_SHIBSP_EXCEPTION(SessionException,SHIBSP_EXCEPTIONAPI(SHIBSP_API),shibsp::agent_exception);
+ DECL_SHIBSP_EXCEPTION(SessionException,SHIBSP_EXCEPTIONAPI(SHIBSP_API),shibsp::AgentException);
DECL_SHIBSP_EXCEPTION(SessionValidationException,SHIBSP_EXCEPTIONAPI(SHIBSP_API),shibsp::SessionException);
#if defined (_MSC_VER)
diff --git a/shibsp/handler/impl/DefaultHandlerConfiguration.cpp b/shibsp/handler/impl/DefaultHandlerConfiguration.cpp
index 5fce82b0..6db19567 100644
--- a/shibsp/handler/impl/DefaultHandlerConfiguration.cpp
+++ b/shibsp/handler/impl/DefaultHandlerConfiguration.cpp
@@ -73,13 +73,17 @@ DefaultHandlerConfiguration::DefaultHandlerConfiguration(const char* pathname)
Category& log = Category::getInstance(SHIBSP_LOGCAT ".HandlerConfiguration");
for (auto& child : m_pt) {
+ if (child.first.empty()) {
+ log.warn("config (%s) skipping handler with no path set", pathname);
+ continue;
+ }
+
boost::optional<string> type = child.second.get_optional<string>("type");
if (!type) {
log.warn("config (%s) skipping handler at %s with no type property", pathname, child.first.c_str());
continue;
}
-
- if (*type == SESSION_INITIATOR_HANDLER && m_sessionInitiator) {
+ else if (*type == SESSION_INITIATOR_HANDLER && m_sessionInitiator) {
throw ConfigurationException("Multiple SessionInitiator handlers were configured, only one is permitted.");
}
diff --git a/shibsp/handler/impl/SessionInitiator.cpp b/shibsp/handler/impl/SessionInitiator.cpp
index 2be3b6f5..7d511869 100644
--- a/shibsp/handler/impl/SessionInitiator.cpp
+++ b/shibsp/handler/impl/SessionInitiator.cpp
@@ -89,10 +89,10 @@ SessionInitiator::SessionInitiator(const ptree& pt, const char* path)
pair<bool,long> SessionInitiator::run(SPRequest& request, bool isHandler) const
{
- try {
- string state, target, handler;
- const char* handlerBaseURL = nullptr;
+ string state, target, handler;
+ const char* handlerBaseURL = nullptr;
+ try {
if (isHandler) {
// Check for a state parameter in the query string.
const char* param = request.getParameter("state");
@@ -171,6 +171,11 @@ pair<bool,long> SessionInitiator::run(SPRequest& request, bool isHandler) const
return unwrapResponse(request, output);
}
catch (exception& ex) {
+ AgentException* agent_ex = dynamic_cast<AgentException*>(&ex);
+ if (agent_ex) {
+ agent_ex->addProperty("handlerType", SESSION_INITIATOR_HANDLER);
+ }
+
// If it's a handler operation, and isPassive is used or returnOnError is set, we trap the error.
if (isHandler) {
bool returnOnError = getBool("isPassive", request, false);
@@ -180,12 +185,12 @@ pair<bool,long> SessionInitiator::run(SPRequest& request, bool isHandler) const
if (returnOnError) {
m_log.warn(ex.what());
- const agent_exception* agent_ex = dynamic_cast<const agent_exception*>(&ex);
- const char* target = agent_ex ? agent_ex->getProperty("target") : nullptr;
- if (target) {
+ const char* error_target = agent_ex ? agent_ex->getProperty("target") : nullptr;
+ // Make sure the target isn't the same as this handler, so avoid a loop.
+ if (error_target && strcmp(error_target, handler.c_str())) {
m_log.info("trapping SessionInitiator failure and returning to target location");
- request.limitRedirect(target);
- return make_pair(true, request.sendRedirect(target));
+ request.limitRedirect(error_target);
+ return make_pair(true, request.sendRedirect(error_target));
}
}
}
diff --git a/shibsp/remoting/impl/AbstractRemotingService.cpp b/shibsp/remoting/impl/AbstractRemotingService.cpp
index 8c7da84a..faf8aaa7 100644
--- a/shibsp/remoting/impl/AbstractRemotingService.cpp
+++ b/shibsp/remoting/impl/AbstractRemotingService.cpp
@@ -46,8 +46,11 @@ DDF AbstractRemotingService::send(const DDF& in) const
const char* event = output.getmember("event").string();
if (event && strcmp(event, "success")) {
- OperationException ex("A remote operation was unsuccessful.");
+ OperationException ex("Remote operation was unsuccessful.");
ex.addProperty("event", event);
+ if (in.name()) {
+ ex.addProperty("operation", in.name());
+ }
const char* target = output.getmember("target").string();
if (target) {
ex.addProperty("target", target);
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list