[cpp-sp] branch main updated: Rework tree processing in ACL impl.
Scott Cantor
cantor.2 at osu.edu
Wed Dec 18 17:34:27 UTC 2024
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=4f55c4ff639895104af0fa55d4f37de371d7f014
The following commit(s) were added to refs/heads/main by this push:
new 4f55c4ff Rework tree processing in ACL impl.
4f55c4ff is described below
commit 4f55c4ff639895104af0fa55d4f37de371d7f014
Author: Scott Cantor <cantor.2 at osu.edu>
AuthorDate: Wed Dec 18 12:34:23 2024 -0500
Rework tree processing in ACL impl.
---
shibsp/impl/XMLAccessControl.cpp | 84 +++++++++++++++++++------------
tests/data/impl/internal-acl-invalid2.xml | 8 +++
tests/impl/XMLAccessControlTests.cpp | 27 +++++++---
tests/util/ReloadableXMLFileTests.cpp | 1 -
4 files changed, 80 insertions(+), 40 deletions(-)
diff --git a/shibsp/impl/XMLAccessControl.cpp b/shibsp/impl/XMLAccessControl.cpp
index de627ee8..d65a7454 100644
--- a/shibsp/impl/XMLAccessControl.cpp
+++ b/shibsp/impl/XMLAccessControl.cpp
@@ -299,14 +299,18 @@ AccessControl::aclresult_t RuleRegex::authorized(const SPRequest& request, const
Operator::Operator(const string& name, const ptree& pt)
{
- if (name == "NOT")
- m_op=OP_NOT;
- else if (name == "AND")
- m_op=OP_AND;
- else if (name == "OR")
- m_op=OP_OR;
- else
- throw ConfigurationException("Unrecognized access control rule type");
+ if (name == "NOT") {
+ m_op = OP_NOT;
+ }
+ else if (name == "AND") {
+ m_op = OP_AND;
+ }
+ else if (name == "OR") {
+ m_op = OP_OR;
+ }
+ else {
+ throw ConfigurationException(string("Unrecognized access control operator: ") + name);
+ }
for (const auto& child : pt) {
if (child.first == RULE_PROP_PATH) {
@@ -315,7 +319,7 @@ Operator::Operator(const string& name, const ptree& pt)
else if (child.first == RULE_REGEX_PROP_PATH) {
m_operands.push_back(unique_ptr<AccessControl>(new RuleRegex(child.second)));
}
- else {
+ else if (child.first != "<xmlattr>") {
m_operands.push_back(unique_ptr<AccessControl>(new Operator(child.first, child.second)));
}
}
@@ -370,11 +374,8 @@ unique_ptr<AccessControl> XMLAccessControl::processChild(const string& name, con
else if (name == RULE_REGEX_PROP_PATH) {
return unique_ptr<AccessControl>(new RuleRegex(pt));
}
- else if (name != "<xmlattr>") {
- return unique_ptr<AccessControl>(new Operator(name, pt));
- }
else {
- return nullptr;
+ return unique_ptr<AccessControl>(new Operator(name, pt));
}
}
@@ -386,29 +387,46 @@ pair<bool,ptree*> XMLAccessControl::load() noexcept
return raw;
}
- // If we own it, wrap it, but we don't retain use of it.
- unique_ptr<ptree> treejanitor(raw.first ? raw.second : nullptr);
-
- // This is tentative and almost certainly wrong due to the way the XML
- // worked in the original config.
-
- // In the inline case, there should be a child element named
- // AccessControl so we need to step down one level.
- unique_ptr<AccessControl> authz;
- const auto& child = raw.second->front();
- if (child.first == ACCESS_CONTROL_PROP_PATH) {
- const auto& child2 = child.second.front();
- authz = processChild(child2.first, child2.second);
- } else {
- authz = processChild(child.first, child.second);
- }
+ try {
+ // If we own it, wrap it, but we don't retain use of it.
+ unique_ptr<ptree> treejanitor(raw.first ? raw.second : nullptr);
- if (authz) {
- // Perform the swap inside a lock.
+ unique_ptr<AccessControl> authz;
+
+ // We have to skip the <xmlattr> node if it appears.
+ // In the inline case, there should be a child element named
+ // AccessControl so we need to step down one level (and again
+ // skip the <xmlattr> node.
+
+ for (const auto& child : *raw.second) {
+ if (child.first == "<xmlattr>") {
+ continue;
+ }
+ else if (child.first == ACCESS_CONTROL_PROP_PATH) {
+ for (const auto& child2 : child.second) {
+ if (child2.first == "<xmlattr>") {
+ continue;
+ }
+ else {
+ authz = processChild(child2.first, child2.second);
+ }
+ }
+ }
+ else {
+ authz = processChild(child.first, child.second);
+ }
+ }
+
+ if (authz) {
+ // Perform the swap inside a lock.
#ifdef HAVE_CXX14
- unique_lock<ReloadableXMLFile> locker(*this);
+ unique_lock<ReloadableXMLFile> locker(*this);
#endif
- m_rootAuthz.swap(authz);
+ m_rootAuthz.swap(authz);
+ return make_pair(false, raw.second);
+ }
+ } catch (const std::exception& e) {
+ m_log.error("exception processing XML configuration: %s", e.what());
}
return make_pair(false, nullptr);
diff --git a/tests/data/impl/internal-acl-invalid2.xml b/tests/data/impl/internal-acl-invalid2.xml
new file mode 100644
index 00000000..3f5496cb
--- /dev/null
+++ b/tests/data/impl/internal-acl-invalid2.xml
@@ -0,0 +1,8 @@
+<AccessControlProvider type="XML">
+ <AccessControl>
+ <AND>
+ <Rule require="affiliation">foo</Rule>
+ <Foo/>
+ </AND>
+ </AccessControl>
+</AccessControlProvider>
diff --git a/tests/impl/XMLAccessControlTests.cpp b/tests/impl/XMLAccessControlTests.cpp
index d7608123..f7d12fa9 100644
--- a/tests/impl/XMLAccessControlTests.cpp
+++ b/tests/impl/XMLAccessControlTests.cpp
@@ -98,7 +98,6 @@ class exceptionCheck {
public:
exceptionCheck(const string& msg) : m_msg(msg) {}
bool check_message(const exception& e) {
- cout << e.what() << endl;
return m_msg.compare(e.what()) == 0;
}
private:
@@ -126,8 +125,6 @@ struct External_Invalid_Fixture : public BaseFixture
External_Invalid_Fixture() {
xml_parser::read_xml(data_path + "external-acl-badxml.xml", tree, xml_parser::no_comments|xml_parser::trim_whitespace);
}
- ~External_Invalid_Fixture() {
- }
ptree tree;
};
@@ -151,8 +148,6 @@ struct Inline_Invalid_Fixture : public BaseFixture
Inline_Invalid_Fixture() {
xml_parser::read_xml(data_path + "internal-acl-invalid.xml", tree, xml_parser::no_comments|xml_parser::trim_whitespace);
}
- ~Inline_Invalid_Fixture() {
- }
ptree tree;
};
@@ -167,10 +162,30 @@ BOOST_FIXTURE_TEST_CASE(XMLAccessControl_inline_invalid, Inline_Invalid_Fixture)
ConfigurationException, checker.check_message);
}
+/////////////
+// Inline ACL content that has a bad internal element.
/////////////
-/*
+struct Inline_InvalidInternal_Fixture : public BaseFixture
+{
+ Inline_InvalidInternal_Fixture() {
+ xml_parser::read_xml(data_path + "internal-acl-invalid2.xml", tree, xml_parser::no_comments|xml_parser::trim_whitespace);
+ }
+ ptree tree;
+};
+
+BOOST_FIXTURE_TEST_CASE(XMLAccessControl_inline_invalid_internal, Inline_InvalidInternal_Fixture)
+{
+ BOOST_CHECK_EQUAL(tree.size(), 1);
+
+ exceptionCheck checker("Initial AccessControl configuration was invalid.");
+ BOOST_CHECK_EXCEPTION(AgentConfig::getConfig().AccessControlManager.newPlugin(
+ tree.front().second.get<string>("<xmlattr>.type").c_str(), tree.front().second, true),
+ ConfigurationException, checker.check_message);
+}
+
+/*
struct Inline_Valid_Fixture : public BaseFixture
{
Inline_Valid_Fixture() {
diff --git a/tests/util/ReloadableXMLFileTests.cpp b/tests/util/ReloadableXMLFileTests.cpp
index 8befae50..f9e8169e 100644
--- a/tests/util/ReloadableXMLFileTests.cpp
+++ b/tests/util/ReloadableXMLFileTests.cpp
@@ -103,7 +103,6 @@ class exceptionCheck {
public:
exceptionCheck(const string& msg) : m_msg(msg) {}
bool check_message(const exception& e) {
- cout << e.what() << endl;
return m_msg.compare(e.what()) == 0;
}
private:
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list