From 48946faf0cc1aacda4168766b23e7ae46e8d9437 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 24 Dec 2025 16:51:52 -0800 Subject: [PATCH 01/21] Refactor: libcrmcommon: Unindent a block in xml_acl_filtered_copy() Don't do other best practices yet. Those will be in upcoming commits. Signed-off-by: Reid Wahl --- lib/common/acl.c | 55 +++++++++++++++++++++++------------------------- 1 file changed, 26 insertions(+), 29 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 039e00968a4..87642b29913 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -500,44 +500,41 @@ xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, docpriv = target->doc->_private; for(aIter = docpriv->acls; aIter != NULL && target; aIter = aIter->next) { - int max = 0; xml_acl_t *acl = aIter->data; + xmlXPathObject *xpathObj = NULL; + int max = 0; + int lpc = 0; - if (acl->mode != pcmk__xf_acl_deny) { - /* Nothing to do */ + if ((acl->mode != pcmk__xf_acl_deny) || (acl->xpath == NULL)) { + continue; + } - } else if (acl->xpath) { - int lpc = 0; - xmlXPathObject *xpathObj = pcmk__xpath_search(target->doc, - acl->xpath); + xpathObj = pcmk__xpath_search(target->doc, acl->xpath); + max = pcmk__xpath_num_results(xpathObj); - max = pcmk__xpath_num_results(xpathObj); - for(lpc = 0; lpc < max; lpc++) { - xmlNode *match = pcmk__xpath_result(xpathObj, lpc); + for(lpc = 0; lpc < max; lpc++) { + xmlNode *match = pcmk__xpath_result(xpathObj, lpc); - if (match == NULL) { - continue; - } + if (match == NULL) { + continue; + } - // @COMPAT See COMPAT comment in pcmk__apply_acl() - match = pcmk__xpath_match_element(match); - if (match == NULL) { - continue; - } + // @COMPAT See COMPAT comment in pcmk__apply_acl() + match = pcmk__xpath_match_element(match); + if (match == NULL) { + continue; + } - if (!purge_xml_attributes(match) && (match == target)) { - pcmk__trace("ACLs deny user '%s' access to entire XML " - "document", - user); - xmlXPathFreeObject(xpathObj); - return true; - } + if (!purge_xml_attributes(match) && (match == target)) { + pcmk__trace("ACLs deny user '%s' access to entire XML document", + user); + xmlXPathFreeObject(xpathObj); + return true; } - pcmk__trace("ACLs deny user '%s' access to %s (%d %s)", user, - acl->xpath, max, - pcmk__plural_alt(max, "match", "matches")); - xmlXPathFreeObject(xpathObj); } + pcmk__trace("ACLs deny user '%s' access to %s (%d %s)", user, + acl->xpath, max, pcmk__plural_alt(max, "match", "matches")); + xmlXPathFreeObject(xpathObj); } if (!purge_xml_attributes(target)) { From f29303635cb77975d935445488b549b5c12f3b72 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 24 Dec 2025 17:00:03 -0800 Subject: [PATCH 02/21] Refactor: libcrmcommon: Rename some variables in xml_acl_filtered_copy() Also use const for the GList iterator and the xml_acl_t variable. Signed-off-by: Reid Wahl --- lib/common/acl.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 87642b29913..0b25fc7481d 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -479,7 +479,6 @@ bool xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, xmlNode **result) { - GList *aIter = NULL; xmlNode *target = NULL; xml_doc_private_t *docpriv = NULL; @@ -499,21 +498,22 @@ xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, pcmk__enable_acl(acl_source, target, user); docpriv = target->doc->_private; - for(aIter = docpriv->acls; aIter != NULL && target; aIter = aIter->next) { - xml_acl_t *acl = aIter->data; - xmlXPathObject *xpathObj = NULL; - int max = 0; - int lpc = 0; + for (const GList *iter = docpriv->acls; (iter != NULL) && (target != NULL); + iter = iter->next) { + + const xml_acl_t *acl = iter->data; + xmlXPathObject *xpath_obj = NULL; + int num_results = 0; if ((acl->mode != pcmk__xf_acl_deny) || (acl->xpath == NULL)) { continue; } - xpathObj = pcmk__xpath_search(target->doc, acl->xpath); - max = pcmk__xpath_num_results(xpathObj); + xpath_obj = pcmk__xpath_search(target->doc, acl->xpath); + num_results = pcmk__xpath_num_results(xpath_obj); - for(lpc = 0; lpc < max; lpc++) { - xmlNode *match = pcmk__xpath_result(xpathObj, lpc); + for (int i = 0; i < num_results; i++) { + xmlNode *match = pcmk__xpath_result(xpath_obj, i); if (match == NULL) { continue; @@ -528,13 +528,14 @@ xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, if (!purge_xml_attributes(match) && (match == target)) { pcmk__trace("ACLs deny user '%s' access to entire XML document", user); - xmlXPathFreeObject(xpathObj); + xmlXPathFreeObject(xpath_obj); return true; } } - pcmk__trace("ACLs deny user '%s' access to %s (%d %s)", user, - acl->xpath, max, pcmk__plural_alt(max, "match", "matches")); - xmlXPathFreeObject(xpathObj); + pcmk__trace("ACLs deny user '%s' access to %s (%d match%s)", user, + acl->xpath, num_results, + pcmk__plural_alt(num_results, "", "es")); + xmlXPathFreeObject(xpath_obj); } if (!purge_xml_attributes(target)) { From 2c7c8ee326010d9e5445aa3eaeacbc160a6063c4 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 26 Jan 2026 23:05:44 -0800 Subject: [PATCH 03/21] Refactor: libcrmcommon: Drop some NULL checks in xml_acl_filtered_copy() pcmk__xml_copy() is guaranteed to return non-NULL if its second argument is non-NULL. We have already ensured that xml is not NULL, and so target is not NULL. Further, nothing in the body of the for loop can set target to NULL. Also drop a trace message that doesn't seem helpful. Signed-off-by: Reid Wahl --- lib/common/acl.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 0b25fc7481d..7e014da7697 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -484,23 +484,17 @@ xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, *result = NULL; if ((xml == NULL) || !pcmk_acl_required(user)) { - pcmk__trace("Not filtering XML because ACLs not required for user '%s'", - user); return false; } pcmk__trace("Filtering XML copy using user '%s' ACLs", user); + target = pcmk__xml_copy(NULL, xml); - if (target == NULL) { - return true; - } + docpriv = target->doc->_private; pcmk__enable_acl(acl_source, target, user); - docpriv = target->doc->_private; - for (const GList *iter = docpriv->acls; (iter != NULL) && (target != NULL); - iter = iter->next) { - + for (const GList *iter = docpriv->acls; iter != NULL; iter = iter->next) { const xml_acl_t *acl = iter->data; xmlXPathObject *xpath_obj = NULL; int num_results = 0; From 894bcdbd8aa39948ef9696aa8a24f30978c6ac9d Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 26 Jan 2026 23:13:07 -0800 Subject: [PATCH 04/21] Refactor: libcrmcommon: Use g_clear_pointer() in xml_acl_filtered_copy() Signed-off-by: Reid Wahl --- lib/common/acl.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 7e014da7697..02bf49d3f54 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -538,8 +538,7 @@ xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, } if (docpriv->acls) { - g_list_free_full(docpriv->acls, free_acl); - docpriv->acls = NULL; + g_clear_pointer(&docpriv->acls, pcmk__free_acls); } else { pcmk__trace("User '%s' without ACLs denied access to entire XML " From 61a9ca8a2ab97c5210b6f515abfe6faf444dbffd Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 24 Dec 2025 17:03:32 -0800 Subject: [PATCH 05/21] Refactor: libcrmcommon: Unindent another block of xml_acl_filtered_copy If the "else"/"docpriv->acls == NULL" block was executed, then target was set to NULL, which meant we wouldn't do anything else before returning true. With this change, target cannot be NULL at the end of the function, so we drop the NULL check there. Signed-off-by: Reid Wahl --- lib/common/acl.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 02bf49d3f54..90374af731b 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -487,13 +487,13 @@ xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, return false; } - pcmk__trace("Filtering XML copy using user '%s' ACLs", user); - target = pcmk__xml_copy(NULL, xml); docpriv = target->doc->_private; pcmk__enable_acl(acl_source, target, user); + pcmk__trace("Filtering XML copy using user '%s' ACLs", user); + for (const GList *iter = docpriv->acls; iter != NULL; iter = iter->next) { const xml_acl_t *acl = iter->data; xmlXPathObject *xpath_obj = NULL; @@ -537,21 +537,15 @@ xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, return true; } - if (docpriv->acls) { - g_clear_pointer(&docpriv->acls, pcmk__free_acls); - - } else { + if (docpriv->acls == NULL) { pcmk__trace("User '%s' without ACLs denied access to entire XML " - "document", - user); + "document", user); pcmk__xml_free(target); - target = NULL; - } - - if (target) { - *result = target; + return true; } + g_clear_pointer(&docpriv->acls, pcmk__free_acls); + *result = target; return true; } From 141fb9f029485798ac4507cb4056f2db71c4c35e Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 24 Dec 2025 17:44:17 -0800 Subject: [PATCH 06/21] Refactor: libcrmcommon: pcmk__xe_first_child() in purge_xml_attributes() Only element nodes have attributes. Signed-off-by: Reid Wahl --- lib/common/acl.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 90374af731b..5986d5990b4 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -449,12 +449,15 @@ purge_xml_attributes(xmlNode *xml) pcmk__xa_remove(tmp, true); } - child = pcmk__xml_first_child(xml); - while ( child != NULL ) { + child = pcmk__xe_first_child(xml, NULL, NULL, NULL); + while (child != NULL) { xmlNode *tmp = child; - child = pcmk__xml_next(child); - readable_children |= purge_xml_attributes(tmp); + child = pcmk__xe_next(child, NULL); + + if (purge_xml_attributes(tmp)) { + readable_children = true; + } } if (!readable_children) { From 61fe1f07c581fa3cdb84dc688333fc75949ab918 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 24 Dec 2025 17:37:06 -0800 Subject: [PATCH 07/21] Refactor: libcrmcommon: Drop xIter in purge_xml_attributes() Call pcmk__xe_remove_matching_attrs(), which is made for this purpose. Write a simple match function to ensure we only remove non-ID attributes. Signed-off-by: Reid Wahl --- lib/common/acl.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 5986d5990b4..d5a130e30ee 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -411,6 +411,24 @@ test_acl_mode(enum pcmk__xml_flags allowed, enum pcmk__xml_flags requested) return false; } +/*! + * \internal + * \brief Check whether an XML attribute's name is not \c PCMK_XA_ID + * + * \param[in] attr Attribute to check + * \param[in] user_data Ignored + * + * \return \c true if the attribute's name is not \c PCMK_XA_ID, or \c false + * otherwise + * + * \note This is compatible with \c pcmk__xe_remove_matching_attrs(). + */ +static bool +attr_is_not_id(xmlAttr *attr, void *user_data) +{ + return !pcmk__str_eq((const char *) attr->name, PCMK_XA_ID, pcmk__str_none); +} + /*! * \internal * \brief Rid XML tree of all unreadable nodes and node properties @@ -426,7 +444,6 @@ static bool purge_xml_attributes(xmlNode *xml) { xmlNode *child = NULL; - xmlAttr *xIter = NULL; bool readable_children = false; xml_node_private_t *nodepriv = xml->_private; @@ -436,18 +453,7 @@ purge_xml_attributes(xmlNode *xml) return true; } - xIter = xml->properties; - while (xIter != NULL) { - xmlAttr *tmp = xIter; - const char *prop_name = (const char *)xIter->name; - - xIter = xIter->next; - if (strcmp(prop_name, PCMK_XA_ID) == 0) { - continue; - } - - pcmk__xa_remove(tmp, true); - } + pcmk__xe_remove_matching_attrs(xml, true, attr_is_not_id, NULL); child = pcmk__xe_first_child(xml, NULL, NULL, NULL); while (child != NULL) { From 8ee4094c7ddb3c154d121c93d199dd7c2aa0a5d5 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 24 Dec 2025 18:15:50 -0800 Subject: [PATCH 08/21] Refactor: libcrmcommon: Functionize applying a single ACL And rename pcmk__apply_acl() to pcmk__apply_acls(). Signed-off-by: Reid Wahl --- lib/common/acl.c | 147 +++++++++++++++++---------------- lib/common/crmcommon_private.h | 2 +- lib/common/xml.c | 4 +- lib/common/xml_element.c | 2 +- 4 files changed, 80 insertions(+), 75 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index d5a130e30ee..56fd248e909 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -214,13 +214,82 @@ acl_to_text(enum pcmk__xml_flags flags) return "none"; } +static void +apply_acl(xmlDoc *doc, const xml_acl_t *acl) +{ + xml_node_private_t *nodepriv = NULL; + xmlXPathObject *xpath_obj = pcmk__xpath_search(doc, acl->xpath); + int num_results = pcmk__xpath_num_results(xpath_obj); + + for (int i = 0; i < num_results; i++) { + xmlNode *match = pcmk__xpath_result(xpath_obj, i); + + if (match == NULL) { + continue; + } + + /* @COMPAT If the ACL's XPath matches a node that is neither an element + * nor a document, we apply the ACL to the parent element rather than to + * the matched node. For example, if the XPath matches a "score" + * attribute, then it applies to every element that contains a "score" + * attribute. That is, the XPath expression "//@score" matches all + * attributes named "score", but we apply the ACL to all elements + * containing such an attribute. + * + * This behavior is incorrect from an XPath standpoint and is thus + * confusing and counterintuitive. The correct way to match all elements + * containing a "score" attribute is to use an XPath predicate: + * "// *[@score]". (Space inserted after slashes so that GCC doesn't + * throw an error about nested comments.) + * + * Additionally, if an XPath expression matches the entire document (for + * example, "/"), then the ACL applies to the document's root element if + * it exists. + * + * These behaviors should be changed so that the ACL applies to the + * nodes matched by the XPath expression, or so that it doesn't apply at + * all if applying an ACL to an attribute doesn't make sense. + * + * Unfortunately, we document in Pacemaker Explained that matching + * attributes is a valid way to match elements: "Attributes may be + * specified in the XPath to select particular elements, but the + * permissions apply to the entire element." + * + * So we have to keep this behavior at least until a compatibility + * break. Even then, it's not feasible in the general case to transform + * such XPath expressions using XSLT. + */ + match = pcmk__xpath_match_element(match); + if (match == NULL) { + continue; + } + + nodepriv = match->_private; + pcmk__set_xml_flags(nodepriv, acl->mode); + + // Build a GString only if tracing is enabled + pcmk__if_tracing( + { + GString *path = pcmk__element_xpath(match); + pcmk__trace("Applying %s ACL to %s matched by %s", + acl_to_text(acl->mode), path->str, acl->xpath); + g_string_free(path, TRUE); + }, + {} + ); + } + + pcmk__trace("Applied %s ACL %s (%d match%s)", acl_to_text(acl->mode), + acl->xpath, num_results, + pcmk__plural_alt(num_results, "", "es")); + xmlXPathFreeObject(xpath_obj); +} + void -pcmk__apply_acl(xmlNode *xml) +pcmk__apply_acls(xmlNode *xml) { GList *aIter = NULL; xml_doc_private_t *docpriv = NULL; - xml_node_private_t *nodepriv = NULL; - xmlXPathObject *xpathObj = NULL; pcmk__assert(xml != NULL); @@ -233,73 +302,9 @@ pcmk__apply_acl(xmlNode *xml) } for (aIter = docpriv->acls; aIter != NULL; aIter = aIter->next) { - int max = 0, lpc = 0; xml_acl_t *acl = aIter->data; - xpathObj = pcmk__xpath_search(xml->doc, acl->xpath); - max = pcmk__xpath_num_results(xpathObj); - - for (lpc = 0; lpc < max; lpc++) { - xmlNode *match = pcmk__xpath_result(xpathObj, lpc); - - if (match == NULL) { - continue; - } - - /* @COMPAT If the ACL's XPath matches a node that is neither an - * element nor a document, we apply the ACL to the parent element - * rather than to the matched node. For example, if the XPath - * matches a "score" attribute, then it applies to every element - * that contains a "score" attribute. That is, the XPath expression - * "//@score" matches all attributes named "score", but we apply the - * ACL to all elements containing such an attribute. - * - * This behavior is incorrect from an XPath standpoint and is thus - * confusing and counterintuitive. The correct way to match all - * elements containing a "score" attribute is to use an XPath - * predicate: "// *[@score]". (Space inserted after slashes so that - * GCC doesn't throw an error about nested comments.) - * - * Additionally, if an XPath expression matches the entire document - * (for example, "/"), then the ACL applies to the document's root - * element if it exists. - * - * These behaviors should be changed so that the ACL applies to the - * nodes matched by the XPath expression, or so that it doesn't - * apply at all if applying an ACL to an attribute doesn't make - * sense. - * - * Unfortunately, we document in Pacemaker Explained that matching - * attributes is a valid way to match elements: "Attributes may be - * specified in the XPath to select particular elements, but the - * permissions apply to the entire element." - * - * So we have to keep this behavior at least until a compatibility - * break. Even then, it's not feasible in the general case to - * transform such XPath expressions using XSLT. - */ - match = pcmk__xpath_match_element(match); - if (match == NULL) { - continue; - } - - nodepriv = match->_private; - pcmk__set_xml_flags(nodepriv, acl->mode); - - // Build a GString only if tracing is enabled - pcmk__if_tracing( - { - GString *path = pcmk__element_xpath(match); - pcmk__trace("Applying %s ACL to %s matched by %s", - acl_to_text(acl->mode), path->str, acl->xpath); - g_string_free(path, TRUE); - }, - {} - ); - } - pcmk__trace("Applied %s ACL %s (%d match%s)", acl_to_text(acl->mode), - acl->xpath, max, ((max == 1)? "" : "es")); - xmlXPathFreeObject(xpathObj); + apply_acl(xml->doc, acl); } } @@ -387,7 +392,7 @@ pcmk__enable_acl(xmlNode *acl_source, xmlNode *target, const char *user) } pcmk__unpack_acl(acl_source, target, user); pcmk__xml_doc_set_flags(target->doc, pcmk__xf_acl_enabled); - pcmk__apply_acl(target); + pcmk__apply_acls(target); } static inline bool @@ -522,7 +527,7 @@ xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, continue; } - // @COMPAT See COMPAT comment in pcmk__apply_acl() + // @COMPAT See COMPAT comment in pcmk__apply_acls() match = pcmk__xpath_match_element(match); if (match == NULL) { continue; @@ -692,7 +697,7 @@ xml_acl_disable(xmlNode *xml) xml_doc_private_t *docpriv = xml->doc->_private; /* Catch anything that was created but shouldn't have been */ - pcmk__apply_acl(xml); + pcmk__apply_acls(xml); pcmk__apply_creation_acl(xml, false); pcmk__clear_xml_flags(docpriv, pcmk__xf_acl_enabled); } diff --git a/lib/common/crmcommon_private.h b/lib/common/crmcommon_private.h index cb12b69c0a0..2cd98115381 100644 --- a/lib/common/crmcommon_private.h +++ b/lib/common/crmcommon_private.h @@ -150,7 +150,7 @@ G_GNUC_INTERNAL bool pcmk__is_user_in_group(const char *user, const char *group); G_GNUC_INTERNAL -void pcmk__apply_acl(xmlNode *xml); +void pcmk__apply_acls(xmlNode *xml); G_GNUC_INTERNAL void pcmk__apply_creation_acl(xmlNode *xml, bool check_top); diff --git a/lib/common/xml.c b/lib/common/xml.c index 772ef3d4f2e..c71fb1846d0 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -1331,7 +1331,7 @@ mark_child_deleted(xmlNode *old_child, xmlNode *new_parent) pcmk__xml_tree_foreach(candidate, pcmk__xml_reset_node_flags, NULL); // free_xml_with_position() will check whether ACLs allow the deletion - pcmk__apply_acl(xmlDocGetRootElement(candidate->doc)); + pcmk__apply_acls(xmlDocGetRootElement(candidate->doc)); /* Try to remove the child again (which will track it in document's * deleted_objs on success) @@ -1812,7 +1812,7 @@ xml_track_changes(xmlNode *xml, const char *user, xmlNode *acl_source, } pcmk__xml_doc_set_flags(xml->doc, pcmk__xf_acl_enabled); pcmk__unpack_acl(acl_source, xml, user); - pcmk__apply_acl(xml); + pcmk__apply_acls(xml); } } diff --git a/lib/common/xml_element.c b/lib/common/xml_element.c index f39fc5ab4a2..d621b56c6c8 100644 --- a/lib/common/xml_element.c +++ b/lib/common/xml_element.c @@ -758,7 +758,7 @@ replace_node(xmlNode *old, xmlNode *new) if (pcmk__xml_doc_all_flags_set(new->doc, pcmk__xf_tracking)) { // Replaced sections may have included relevant ACLs - pcmk__apply_acl(new); + pcmk__apply_acls(new); } pcmk__xml_mark_changes(old, new); pcmk__xml_free_node(old); From e74074f3e003995d26b792ae9fcf7b6f7471e874 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 24 Dec 2025 18:17:22 -0800 Subject: [PATCH 09/21] Refactor: libcrmcommon: Create GString unconditionally in apply_acl() The pcmk__if_tracing() block makes sense but it feels like premature optimization and makes the code harder to read. I'm not worried about the overhead of allocating and freeing a small GString each time we apply an ACL. Signed-off-by: Reid Wahl --- lib/common/acl.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 56fd248e909..c994508e9d9 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -222,6 +222,7 @@ apply_acl(xmlDoc *doc, const xml_acl_t *acl) int num_results = pcmk__xpath_num_results(xpath_obj); for (int i = 0; i < num_results; i++) { + GString *path = NULL; xmlNode *match = pcmk__xpath_result(xpath_obj, i); if (match == NULL) { @@ -267,16 +268,10 @@ apply_acl(xmlDoc *doc, const xml_acl_t *acl) nodepriv = match->_private; pcmk__set_xml_flags(nodepriv, acl->mode); - // Build a GString only if tracing is enabled - pcmk__if_tracing( - { - GString *path = pcmk__element_xpath(match); - pcmk__trace("Applying %s ACL to %s matched by %s", - acl_to_text(acl->mode), path->str, acl->xpath); - g_string_free(path, TRUE); - }, - {} - ); + path = pcmk__element_xpath(match); + pcmk__trace("Applying %s ACL to %s matched by %s", + acl_to_text(acl->mode), path->str, acl->xpath); + g_string_free(path, TRUE); } pcmk__trace("Applied %s ACL %s (%d match%s)", acl_to_text(acl->mode), From 2bbab9168b6c226bfb5dbad72601b04e907aabbc Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 24 Dec 2025 18:20:28 -0800 Subject: [PATCH 10/21] Refactor: libcrmcommon: Use const in pcmk__apply_acls() Also rename a variable and drop a trace message that seems unhelpful. Signed-off-by: Reid Wahl --- lib/common/acl.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index c994508e9d9..26a9e725b20 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -283,21 +283,17 @@ apply_acl(xmlDoc *doc, const xml_acl_t *acl) void pcmk__apply_acls(xmlNode *xml) { - GList *aIter = NULL; xml_doc_private_t *docpriv = NULL; pcmk__assert(xml != NULL); - docpriv = xml->doc->_private; if (!pcmk__xml_doc_all_flags_set(xml->doc, pcmk__xf_acl_enabled)) { - pcmk__trace("Skipping ACLs for user '%s' because not enabled for this " - "XML", pcmk__s(docpriv->acl_user, "(unknown)")); return; } - for (aIter = docpriv->acls; aIter != NULL; aIter = aIter->next) { - xml_acl_t *acl = aIter->data; + for (const GList *iter = docpriv->acls; iter != NULL; iter = iter->next) { + const xml_acl_t *acl = iter->data; apply_acl(xml->doc, acl); } From dbad067677ee55834dab938014060d0f0c8b6ed0 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 24 Dec 2025 18:28:51 -0800 Subject: [PATCH 11/21] Refactor: libcrmcommon: pcmk__apply_acls() takes an xmlDoc * Signed-off-by: Reid Wahl --- lib/common/acl.c | 14 +++++++------- lib/common/crmcommon_private.h | 2 +- lib/common/xml.c | 4 ++-- lib/common/xml_element.c | 2 +- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 26a9e725b20..11d8f3acc28 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -281,21 +281,21 @@ apply_acl(xmlDoc *doc, const xml_acl_t *acl) } void -pcmk__apply_acls(xmlNode *xml) +pcmk__apply_acls(xmlDoc *doc) { xml_doc_private_t *docpriv = NULL; - pcmk__assert(xml != NULL); - docpriv = xml->doc->_private; + pcmk__assert(doc != NULL); + docpriv = doc->_private; - if (!pcmk__xml_doc_all_flags_set(xml->doc, pcmk__xf_acl_enabled)) { + if (!pcmk__xml_doc_all_flags_set(doc, pcmk__xf_acl_enabled)) { return; } for (const GList *iter = docpriv->acls; iter != NULL; iter = iter->next) { const xml_acl_t *acl = iter->data; - apply_acl(xml->doc, acl); + apply_acl(doc, acl); } } @@ -383,7 +383,7 @@ pcmk__enable_acl(xmlNode *acl_source, xmlNode *target, const char *user) } pcmk__unpack_acl(acl_source, target, user); pcmk__xml_doc_set_flags(target->doc, pcmk__xf_acl_enabled); - pcmk__apply_acls(target); + pcmk__apply_acls(target->doc); } static inline bool @@ -688,7 +688,7 @@ xml_acl_disable(xmlNode *xml) xml_doc_private_t *docpriv = xml->doc->_private; /* Catch anything that was created but shouldn't have been */ - pcmk__apply_acls(xml); + pcmk__apply_acls(xml->doc); pcmk__apply_creation_acl(xml, false); pcmk__clear_xml_flags(docpriv, pcmk__xf_acl_enabled); } diff --git a/lib/common/crmcommon_private.h b/lib/common/crmcommon_private.h index 2cd98115381..e3ae887df89 100644 --- a/lib/common/crmcommon_private.h +++ b/lib/common/crmcommon_private.h @@ -150,7 +150,7 @@ G_GNUC_INTERNAL bool pcmk__is_user_in_group(const char *user, const char *group); G_GNUC_INTERNAL -void pcmk__apply_acls(xmlNode *xml); +void pcmk__apply_acls(xmlDoc *doc); G_GNUC_INTERNAL void pcmk__apply_creation_acl(xmlNode *xml, bool check_top); diff --git a/lib/common/xml.c b/lib/common/xml.c index c71fb1846d0..a54ac94dd38 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -1331,7 +1331,7 @@ mark_child_deleted(xmlNode *old_child, xmlNode *new_parent) pcmk__xml_tree_foreach(candidate, pcmk__xml_reset_node_flags, NULL); // free_xml_with_position() will check whether ACLs allow the deletion - pcmk__apply_acls(xmlDocGetRootElement(candidate->doc)); + pcmk__apply_acls(candidate->doc); /* Try to remove the child again (which will track it in document's * deleted_objs on success) @@ -1812,7 +1812,7 @@ xml_track_changes(xmlNode *xml, const char *user, xmlNode *acl_source, } pcmk__xml_doc_set_flags(xml->doc, pcmk__xf_acl_enabled); pcmk__unpack_acl(acl_source, xml, user); - pcmk__apply_acls(xml); + pcmk__apply_acls(xml->doc); } } diff --git a/lib/common/xml_element.c b/lib/common/xml_element.c index d621b56c6c8..78a2a084963 100644 --- a/lib/common/xml_element.c +++ b/lib/common/xml_element.c @@ -758,7 +758,7 @@ replace_node(xmlNode *old, xmlNode *new) if (pcmk__xml_doc_all_flags_set(new->doc, pcmk__xf_tracking)) { // Replaced sections may have included relevant ACLs - pcmk__apply_acls(new); + pcmk__apply_acls(new->doc); } pcmk__xml_mark_changes(old, new); pcmk__xml_free_node(old); From 3abe51e48581d61f94e048189a82b6d8fb3efbdc Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 24 Dec 2025 18:56:28 -0800 Subject: [PATCH 12/21] Refactor: libcrmcommon: pcmk__xpath_foreach_result in pcmk__apply_acls We drop a trace log in order to do this. To get the number of matches from pcmk__xpath_foreach_result(), we'd have to pass a struct as user data to hold both the ACL itself and the match count. That doesn't seem worth the trouble. We still have the trace message after applying an ACL to a particular match. Signed-off-by: Reid Wahl --- lib/common/acl.c | 103 ++++++++++++++++++++--------------------------- 1 file changed, 44 insertions(+), 59 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 11d8f3acc28..aa4a838edf8 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -215,69 +215,54 @@ acl_to_text(enum pcmk__xml_flags flags) } static void -apply_acl(xmlDoc *doc, const xml_acl_t *acl) +apply_acl(xmlNode *match, void *user_data) { + const xml_acl_t *acl = user_data; xml_node_private_t *nodepriv = NULL; - xmlXPathObject *xpath_obj = pcmk__xpath_search(doc, acl->xpath); - int num_results = pcmk__xpath_num_results(xpath_obj); - - for (int i = 0; i < num_results; i++) { - GString *path = NULL; - xmlNode *match = pcmk__xpath_result(xpath_obj, i); - - if (match == NULL) { - continue; - } - - /* @COMPAT If the ACL's XPath matches a node that is neither an element - * nor a document, we apply the ACL to the parent element rather than to - * the matched node. For example, if the XPath matches a "score" - * attribute, then it applies to every element that contains a "score" - * attribute. That is, the XPath expression "//@score" matches all - * attributes named "score", but we apply the ACL to all elements - * containing such an attribute. - * - * This behavior is incorrect from an XPath standpoint and is thus - * confusing and counterintuitive. The correct way to match all elements - * containing a "score" attribute is to use an XPath predicate: - * "// *[@score]". (Space inserted after slashes so that GCC doesn't - * throw an error about nested comments.) - * - * Additionally, if an XPath expression matches the entire document (for - * example, "/"), then the ACL applies to the document's root element if - * it exists. - * - * These behaviors should be changed so that the ACL applies to the - * nodes matched by the XPath expression, or so that it doesn't apply at - * all if applying an ACL to an attribute doesn't make sense. - * - * Unfortunately, we document in Pacemaker Explained that matching - * attributes is a valid way to match elements: "Attributes may be - * specified in the XPath to select particular elements, but the - * permissions apply to the entire element." - * - * So we have to keep this behavior at least until a compatibility - * break. Even then, it's not feasible in the general case to transform - * such XPath expressions using XSLT. - */ - match = pcmk__xpath_match_element(match); - if (match == NULL) { - continue; - } - - nodepriv = match->_private; - pcmk__set_xml_flags(nodepriv, acl->mode); + GString *path = NULL; - path = pcmk__element_xpath(match); - pcmk__trace("Applying %s ACL to %s matched by %s", - acl_to_text(acl->mode), path->str, acl->xpath); - g_string_free(path, TRUE); + /* @COMPAT If the ACL's XPath matches a node that is neither an element nor + * a document, we apply the ACL to the parent element rather than to the + * matched node. For example, if the XPath matches a "score" attribute, then + * it applies to every element that contains a "score" attribute. That is, + * the XPath expression "//@score" matches all attributes named "score", but + * we apply the ACL to all elements containing such an attribute. + * + * This behavior is incorrect from an XPath standpoint and is thus confusing + * and counterintuitive. The correct way to match all elements containing a + * "score" attribute is to use an XPath predicate: "// *[@score]". (Space + * inserted after slashes so that GCC doesn't throw an error about nested + * comments.) + * + * Additionally, if an XPath expression matches the entire document (for + * example, "/"), then the ACL applies to the document's root element if it + * exists. + * + * These behaviors should be changed so that the ACL applies to the nodes + * matched by the XPath expression, or so that it doesn't apply at all if + * applying an ACL to an attribute doesn't make sense. + * + * Unfortunately, we document in Pacemaker Explained that matching + * attributes is a valid way to match elements: "Attributes may be specified + * in the XPath to select particular elements, but the permissions apply to + * the entire element." + * + * So we have to keep this behavior at least until a compatibility break. + * Even then, it's not feasible in the general case to transform such XPath + * expressions using XSLT. + */ + match = pcmk__xpath_match_element(match); + if (match == NULL) { + return; } - pcmk__trace("Applied %s ACL %s (%d match%s)", acl_to_text(acl->mode), - acl->xpath, num_results, - pcmk__plural_alt(num_results, "", "es")); - xmlXPathFreeObject(xpath_obj); + nodepriv = match->_private; + pcmk__set_xml_flags(nodepriv, acl->mode); + + path = pcmk__element_xpath(match); + pcmk__trace("Applied %s ACL to %s matched by %s", acl_to_text(acl->mode), + path->str, acl->xpath); + g_string_free(path, TRUE); } void @@ -295,7 +280,7 @@ pcmk__apply_acls(xmlDoc *doc) for (const GList *iter = docpriv->acls; iter != NULL; iter = iter->next) { const xml_acl_t *acl = iter->data; - apply_acl(doc, acl); + pcmk__xpath_foreach_result(doc, acl->xpath, apply_acl, (void *) acl); } } From 9bbe4aa260c3e46b6d6dd9f5487dae7efd542226 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 24 Dec 2025 19:05:34 -0800 Subject: [PATCH 13/21] Refactor: libcrmcommon: Assert arg not NULL in pcmk__unpack_acl() And rename to pcmk__unpack_acls(). In general, we assume that an XML node has a non-NULL doc and that its doc has a non-NULL _private field. If we ever want to start checking that (in case a node somehow originated outside of pcmk__xe_create()), we should do it more broadly. The two callers already ensure that target is not NULL. Signed-off-by: Reid Wahl --- lib/common/acl.c | 9 +++------ lib/common/crmcommon_private.h | 2 +- lib/common/xml.c | 2 +- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index aa4a838edf8..61fe0fe702a 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -298,14 +298,11 @@ pcmk__apply_acls(xmlDoc *doc) * \param[in] user Username whose ACLs need to be unpacked */ void -pcmk__unpack_acl(xmlNode *source, xmlNode *target, const char *user) +pcmk__unpack_acls(xmlNode *source, xmlNode *target, const char *user) { xml_doc_private_t *docpriv = NULL; - if ((target == NULL) || (target->doc == NULL) - || (target->doc->_private == NULL)) { - return; - } + pcmk__assert(target != NULL); docpriv = target->doc->_private; if (!pcmk_acl_required(user)) { @@ -366,7 +363,7 @@ pcmk__enable_acl(xmlNode *acl_source, xmlNode *target, const char *user) if (target == NULL) { return; } - pcmk__unpack_acl(acl_source, target, user); + pcmk__unpack_acls(acl_source, target, user); pcmk__xml_doc_set_flags(target->doc, pcmk__xf_acl_enabled); pcmk__apply_acls(target->doc); } diff --git a/lib/common/crmcommon_private.h b/lib/common/crmcommon_private.h index e3ae887df89..b2a25fc0d57 100644 --- a/lib/common/crmcommon_private.h +++ b/lib/common/crmcommon_private.h @@ -144,7 +144,7 @@ G_GNUC_INTERNAL void pcmk__free_acls(GList *acls); G_GNUC_INTERNAL -void pcmk__unpack_acl(xmlNode *source, xmlNode *target, const char *user); +void pcmk__unpack_acls(xmlNode *source, xmlNode *target, const char *user); G_GNUC_INTERNAL bool pcmk__is_user_in_group(const char *user, const char *group); diff --git a/lib/common/xml.c b/lib/common/xml.c index a54ac94dd38..5f0acc97491 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -1811,7 +1811,7 @@ xml_track_changes(xmlNode *xml, const char *user, xmlNode *acl_source, acl_source = xml; } pcmk__xml_doc_set_flags(xml->doc, pcmk__xf_acl_enabled); - pcmk__unpack_acl(acl_source, xml, user); + pcmk__unpack_acls(acl_source, xml, user); pcmk__apply_acls(xml->doc); } } From 66814c99d62cb2e966c9b99c9b190c1c18d1c499 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 24 Dec 2025 19:12:16 -0800 Subject: [PATCH 14/21] Refactor: libcrmcommon: Reduce duplication in pcmk__apply_acls() It is possible that these values won't get used, but fetching and setting them is pretty lightweight. Signed-off-by: Reid Wahl --- lib/common/acl.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 61fe0fe702a..4fea94bab75 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -321,24 +321,16 @@ pcmk__unpack_acls(xmlNode *source, xmlNode *target, const char *user) for (child = pcmk__xe_first_child(acls, NULL, NULL, NULL); child != NULL; child = pcmk__xe_next(child, NULL)) { - if (pcmk__xe_is(child, PCMK_XE_ACL_TARGET)) { - const char *id = pcmk__xe_get(child, PCMK_XA_NAME); - - if (id == NULL) { - id = pcmk__xe_get(child, PCMK_XA_ID); - } + const char *id = pcmk__s(pcmk__xe_get(child, PCMK_XA_NAME), + pcmk__xe_id(child)); + if (pcmk__xe_is(child, PCMK_XE_ACL_TARGET)) { if (id && strcmp(id, user) == 0) { pcmk__debug("Unpacking ACLs for user '%s'", id); docpriv->acls = parse_acl_entry(acls, child, docpriv->acls); } - } else if (pcmk__xe_is(child, PCMK_XE_ACL_GROUP)) { - const char *id = pcmk__xe_get(child, PCMK_XA_NAME); - - if (id == NULL) { - id = pcmk__xe_get(child, PCMK_XA_ID); - } + } else if (pcmk__xe_is(child, PCMK_XE_ACL_GROUP)) { if (id && pcmk__is_user_in_group(user,id)) { pcmk__debug("Unpacking ACLs for group '%s'", id); docpriv->acls = parse_acl_entry(acls, child, docpriv->acls); From be68ef31e9c9202a102314b763b10d7cd6aecfd4 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 24 Dec 2025 19:16:03 -0800 Subject: [PATCH 15/21] Refactor: libcrmcommon: Continue on NULL id in pcmk__unpack_acls() Signed-off-by: Reid Wahl --- lib/common/acl.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 4fea94bab75..c8a605962c5 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -324,14 +324,18 @@ pcmk__unpack_acls(xmlNode *source, xmlNode *target, const char *user) const char *id = pcmk__s(pcmk__xe_get(child, PCMK_XA_NAME), pcmk__xe_id(child)); + if (id == NULL) { + continue; + } + if (pcmk__xe_is(child, PCMK_XE_ACL_TARGET)) { - if (id && strcmp(id, user) == 0) { + if (pcmk__str_eq(id, user, pcmk__str_none)) { pcmk__debug("Unpacking ACLs for user '%s'", id); docpriv->acls = parse_acl_entry(acls, child, docpriv->acls); } } else if (pcmk__xe_is(child, PCMK_XE_ACL_GROUP)) { - if (id && pcmk__is_user_in_group(user,id)) { + if (pcmk__is_user_in_group(user, id)) { pcmk__debug("Unpacking ACLs for group '%s'", id); docpriv->acls = parse_acl_entry(acls, child, docpriv->acls); } From f57f4f36c979fd847bcbe758d68a5c6ae42dcc8c Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 24 Dec 2025 19:47:52 -0800 Subject: [PATCH 16/21] Refactor: libcrmcommon: Reduce some duplication in pcmk__unpack_acls() Or at least try to clarify it a bit, by returning early if not target or group. Signed-off-by: Reid Wahl --- lib/common/acl.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index c8a605962c5..7d4ce761e5c 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -321,25 +321,38 @@ pcmk__unpack_acls(xmlNode *source, xmlNode *target, const char *user) for (child = pcmk__xe_first_child(acls, NULL, NULL, NULL); child != NULL; child = pcmk__xe_next(child, NULL)) { - const char *id = pcmk__s(pcmk__xe_get(child, PCMK_XA_NAME), - pcmk__xe_id(child)); + const bool is_target = pcmk__xe_is(child, PCMK_XE_ACL_TARGET); + const bool is_group = pcmk__xe_is(child, PCMK_XE_ACL_GROUP); + const char *id = NULL; + if (!is_target && !is_group) { + continue; + } + + id = pcmk__s(pcmk__xe_get(child, PCMK_XA_NAME), + pcmk__xe_id(child)); if (id == NULL) { + // Not possible with schema validation enabled continue; } - if (pcmk__xe_is(child, PCMK_XE_ACL_TARGET)) { - if (pcmk__str_eq(id, user, pcmk__str_none)) { - pcmk__debug("Unpacking ACLs for user '%s'", id); - docpriv->acls = parse_acl_entry(acls, child, docpriv->acls); + if (is_target) { + if (!pcmk__str_eq(id, user, pcmk__str_none)) { + continue; } - } else if (pcmk__xe_is(child, PCMK_XE_ACL_GROUP)) { - if (pcmk__is_user_in_group(user, id)) { - pcmk__debug("Unpacking ACLs for group '%s'", id); - docpriv->acls = parse_acl_entry(acls, child, docpriv->acls); + pcmk__debug("Unpacking ACLs for user '%s'", id); + + } else { + if (!pcmk__is_user_in_group(user, id)) { + continue; } + + pcmk__debug("Unpacking ACLs for group '%s' (user '%s')", id, + user); } + + docpriv->acls = parse_acl_entry(acls, child, docpriv->acls); } } } From 186f845373deae50961b21436e9b22d295f0d46b Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 24 Dec 2025 19:51:13 -0800 Subject: [PATCH 17/21] Refactor: libcrmcommon: Unindent loop in pcmk__unpack_acls() Signed-off-by: Reid Wahl --- lib/common/acl.c | 57 ++++++++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 7d4ce761e5c..3f3880b8cf7 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -315,45 +315,46 @@ pcmk__unpack_acls(xmlNode *source, xmlNode *target, const char *user) pcmk__str_update(&(docpriv->acl_user), user); - if (acls) { - xmlNode *child = NULL; + if (acls == NULL) { + return; + } - for (child = pcmk__xe_first_child(acls, NULL, NULL, NULL); - child != NULL; child = pcmk__xe_next(child, NULL)) { + for (const xmlNode *child = pcmk__xe_first_child(acls, NULL, NULL, + NULL); + child != NULL; child = pcmk__xe_next(child, NULL)) { - const bool is_target = pcmk__xe_is(child, PCMK_XE_ACL_TARGET); - const bool is_group = pcmk__xe_is(child, PCMK_XE_ACL_GROUP); - const char *id = NULL; + const bool is_target = pcmk__xe_is(child, PCMK_XE_ACL_TARGET); + const bool is_group = pcmk__xe_is(child, PCMK_XE_ACL_GROUP); + const char *id = NULL; - if (!is_target && !is_group) { - continue; - } + if (!is_target && !is_group) { + continue; + } - id = pcmk__s(pcmk__xe_get(child, PCMK_XA_NAME), - pcmk__xe_id(child)); - if (id == NULL) { - // Not possible with schema validation enabled + id = pcmk__s(pcmk__xe_get(child, PCMK_XA_NAME), + pcmk__xe_id(child)); + if (id == NULL) { + // Not possible with schema validation enabled + continue; + } + + if (is_target) { + if (!pcmk__str_eq(id, user, pcmk__str_none)) { continue; } - if (is_target) { - if (!pcmk__str_eq(id, user, pcmk__str_none)) { - continue; - } - - pcmk__debug("Unpacking ACLs for user '%s'", id); + pcmk__debug("Unpacking ACLs for user '%s'", id); - } else { - if (!pcmk__is_user_in_group(user, id)) { - continue; - } - - pcmk__debug("Unpacking ACLs for group '%s' (user '%s')", id, - user); + } else { + if (!pcmk__is_user_in_group(user, id)) { + continue; } - docpriv->acls = parse_acl_entry(acls, child, docpriv->acls); + pcmk__debug("Unpacking ACLs for group '%s' (user '%s')", id, + user); } + + docpriv->acls = parse_acl_entry(acls, child, docpriv->acls); } } } From 71354e02817ff2d49e4a659da47a304e956ce519 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 24 Dec 2025 19:57:20 -0800 Subject: [PATCH 18/21] Refactor: libcrmcommon: Unindent more of pcmk__unpack_acls() For simplicity, this drops a trace message that doesn't seem helpful. Also, we drop the explicit NULL check of acls. If it's NULL, we won't enter the loop anyway, because its first child will be NULL. Signed-off-by: Reid Wahl --- lib/common/acl.c | 68 +++++++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 39 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 3f3880b8cf7..49a13ad05d6 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -301,61 +301,51 @@ void pcmk__unpack_acls(xmlNode *source, xmlNode *target, const char *user) { xml_doc_private_t *docpriv = NULL; + xmlNode *acls = NULL; pcmk__assert(target != NULL); docpriv = target->doc->_private; - if (!pcmk_acl_required(user)) { - pcmk__trace("Not unpacking ACLs because not required for user '%s'", - user); + if (!pcmk_acl_required(user) || (docpriv->acls != NULL)) { + return; + } - } else if (docpriv->acls == NULL) { - xmlNode *acls = pcmk__xpath_find_one(source->doc, "//" PCMK_XE_ACLS, - PCMK__LOG_NEVER); + pcmk__str_update(&docpriv->acl_user, user); - pcmk__str_update(&(docpriv->acl_user), user); + acls = pcmk__xpath_find_one(source->doc, "//" PCMK_XE_ACLS, + PCMK__LOG_NEVER); - if (acls == NULL) { - return; - } + for (const xmlNode *child = pcmk__xe_first_child(acls, NULL, NULL, NULL); + child != NULL; child = pcmk__xe_next(child, NULL)) { - for (const xmlNode *child = pcmk__xe_first_child(acls, NULL, NULL, - NULL); - child != NULL; child = pcmk__xe_next(child, NULL)) { + const bool is_target = pcmk__xe_is(child, PCMK_XE_ACL_TARGET); + const bool is_group = pcmk__xe_is(child, PCMK_XE_ACL_GROUP); + const char *id = NULL; - const bool is_target = pcmk__xe_is(child, PCMK_XE_ACL_TARGET); - const bool is_group = pcmk__xe_is(child, PCMK_XE_ACL_GROUP); - const char *id = NULL; + if (!is_target && !is_group) { + continue; + } - if (!is_target && !is_group) { - continue; - } + id = pcmk__s(pcmk__xe_get(child, PCMK_XA_NAME), pcmk__xe_id(child)); + if (id == NULL) { + // Not possible with schema validation enabled + continue; + } - id = pcmk__s(pcmk__xe_get(child, PCMK_XA_NAME), - pcmk__xe_id(child)); - if (id == NULL) { - // Not possible with schema validation enabled + if (is_target) { + if (!pcmk__str_eq(id, user, pcmk__str_none)) { continue; } + pcmk__debug("Unpacking ACLs for user '%s'", id); - if (is_target) { - if (!pcmk__str_eq(id, user, pcmk__str_none)) { - continue; - } - - pcmk__debug("Unpacking ACLs for user '%s'", id); - - } else { - if (!pcmk__is_user_in_group(user, id)) { - continue; - } - - pcmk__debug("Unpacking ACLs for group '%s' (user '%s')", id, - user); + } else { + if (!pcmk__is_user_in_group(user, id)) { + continue; } - - docpriv->acls = parse_acl_entry(acls, child, docpriv->acls); + pcmk__debug("Unpacking ACLs for group '%s' (user '%s')", id, user); } + + docpriv->acls = parse_acl_entry(acls, child, docpriv->acls); } } From 42c1f9efff6612cdf92b2fd756d62205ae7931e4 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 25 Dec 2025 02:02:28 -0800 Subject: [PATCH 19/21] Refactor: libcrmcommon: parse_acl_entry() specifies element in iterators Use the second argument of pcmk__xe_first_child() and pcmk__xe_next() to specify what type of element to get (PCMK_XE_ACL_ROLE). Also use const, unindent a block with pcmk__trace() in it, and use pcmk__xe_id(). Signed-off-by: Reid Wahl --- lib/common/acl.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 49a13ad05d6..b8cc416223a 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -150,25 +150,21 @@ parse_acl_entry(const xmlNode *acl_top, const xmlNode *acl_entry, GList *acls) continue; } - for (xmlNode *role = pcmk__xe_first_child(acl_top, NULL, NULL, - NULL); - role != NULL; role = pcmk__xe_next(role, NULL)) { + for (const xmlNode *role = pcmk__xe_first_child(acl_top, + PCMK_XE_ACL_ROLE, + NULL, NULL); + role != NULL; role = pcmk__xe_next(role, PCMK_XE_ACL_ROLE)) { - const char *role_id = NULL; + const char *role_id = pcmk__xe_id(role); - if (!pcmk__xe_is(role, PCMK_XE_ACL_ROLE)) { + if (!pcmk__str_eq(ref_role, role_id, pcmk__str_none)) { continue; } - role_id = pcmk__xe_get(role, PCMK_XA_ID); - - if (pcmk__str_eq(ref_role, role_id, pcmk__str_none)) { - pcmk__trace("Unpacking referenced role '%s' in <%s> " - "element", - role_id, acl_entry->name); - acls = parse_acl_entry(acl_top, role, acls); - break; - } + pcmk__trace("Unpacking referenced role '%s' in <%s> element", + role_id, acl_entry->name); + acls = parse_acl_entry(acl_top, role, acls); + break; } } } From 0fc264b26272d28aad2a64d48552cbbf07fd8596 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 25 Dec 2025 11:37:47 -0800 Subject: [PATCH 20/21] Refactor: libcrmcommon: Functionize unpacking ACL permission ...as well as the parsing of an ACL mode. Signed-off-by: Reid Wahl --- lib/common/acl.c | 91 ++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 72 insertions(+), 19 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index b8cc416223a..a79bfa8fb0d 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -47,6 +47,32 @@ pcmk__free_acls(GList *acls) g_list_free_full(acls, free_acl); } +/*! + * \internal + * \brief Parse an ACL mode from a string + * + * \param[in] text String to parse + * + * \return ACL mode corresponding to \p text + */ +static enum pcmk__xml_flags +parse_acl_mode(const char *text) +{ + if (pcmk__str_eq(text, PCMK_VALUE_READ, pcmk__str_none)) { + return pcmk__xf_acl_read; + } + + if (pcmk__str_eq(text, PCMK_VALUE_WRITE, pcmk__str_none)) { + return pcmk__xf_acl_write; + } + + if (pcmk__str_eq(text, PCMK_VALUE_DENY, pcmk__str_none)) { + return pcmk__xf_acl_deny; + } + + return pcmk__xf_none; +} + static GList * create_acl(const xmlNode *xml, GList *acls, enum pcmk__xml_flags mode) { @@ -101,6 +127,51 @@ create_acl(const xmlNode *xml, GList *acls, enum pcmk__xml_flags mode) return g_list_append(acls, acl); } +/*! + * \internal + * \brief Unpack a \c PCMK_XE_ACL_PERMISSION element to an \c xml_acl_t + * + * Append the new \c xml_acl_t object to a list. + * + * \param[in] xml Permission element to unpack + * \param[in,out] acls List of ACLs to append to (\c NULL to start a new list) + * + * \return On success, \p acls with the new item appended, or a new list + * containing only the new item if \p acls is \c NULL. On failure, + * \p acls (unmodified). + * + * \note The caller is responsible for freeing the return value using + * \c pcmk__free_acls(). + */ +static GList * +unpack_acl_permission(const xmlNode *xml, GList *acls) +{ + // ID unset is not possible with schema validation enabled + const char *id = pcmk__s(pcmk__xe_id(xml), "(no ID)"); + const char *type = (const char *) xml->name; + const char *kind_s = pcmk__xe_get(xml, PCMK_XA_KIND); + enum pcmk__xml_flags kind = pcmk__xf_none; + + if (kind_s == NULL) { + // Not possible with schema validation enabled + pcmk__warn("Ignoring <%s> element %s with no " PCMK_XA_KIND " " + "attribute", type, id); + return acls; + } + + kind = parse_acl_mode(kind_s); + if (kind == pcmk__xf_none) { + pcmk__warn("Ignoring <%s> element %s with unknown ACL kind '%s'", type, + id, kind_s); + return acls; + } + + pcmk__trace("Unpacking <%s> element %s with " PCMK_XA_KIND "='%s'", type, + id, kind_s); + + return create_acl(xml, acls, kind); +} + /*! * \internal * \brief Unpack a user, group, or role subtree of the ACLs section @@ -121,25 +192,7 @@ parse_acl_entry(const xmlNode *acl_top, const xmlNode *acl_entry, GList *acls) child != NULL; child = pcmk__xe_next(child, NULL)) { if (pcmk__xe_is(child, PCMK_XE_ACL_PERMISSION)) { - const char *kind = pcmk__xe_get(child, PCMK_XA_KIND); - - pcmk__assert(kind != NULL); - pcmk__trace("Unpacking <" PCMK_XE_ACL_PERMISSION "> element of " - "kind '%s'", - kind); - - if (pcmk__str_eq(kind, PCMK_VALUE_READ, pcmk__str_none)) { - acls = create_acl(child, acls, pcmk__xf_acl_read); - - } else if (pcmk__str_eq(kind, PCMK_VALUE_WRITE, pcmk__str_none)) { - acls = create_acl(child, acls, pcmk__xf_acl_write); - - } else if (pcmk__str_eq(kind, PCMK_VALUE_DENY, pcmk__str_none)) { - acls = create_acl(child, acls, pcmk__xf_acl_deny); - - } else { - pcmk__warn("Ignoring unknown ACL kind '%s'", kind); - } + acls = unpack_acl_permission(child, acls); } else if (pcmk__xe_is(child, PCMK_XE_ROLE)) { const char *ref_role = pcmk__xe_get(child, PCMK_XA_ID); From 9d96604f307276a556fac982cc8626adac4feace Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 25 Dec 2025 12:11:57 -0800 Subject: [PATCH 21/21] Log: libcrmcommon: Set config warnings and errors for acl_permission An acl_permission element's ID is currently used only for logging. So we can be permissive and allow this. It seems that our usual approach to things that the schema doesn't allow is to continue processing them if it's possible (for example, if it doesn't result in a broken reference). So warn here. For missing or invalid "kind" attribute, we have to ignore the element, so set a config error. It doesn't look as if we're very consistent about when we set warnings vs. errors, so I'm just doing what feels like it makes the most sense. We could just as well call all of these warnings or call all of these errors. Also, log the parent type and ID parenthetically for errors other than missing ID. If we proceeded with unpacking an acl_permission element without an ID, we want any further log messages to have some identifying information about where or what the acl_permission element is. It doesn't hurt to log this even if the acl_permission's ID is set. Signed-off-by: Reid Wahl --- lib/common/acl.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index a79bfa8fb0d..30e8144dca0 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -146,28 +146,42 @@ create_acl(const xmlNode *xml, GList *acls, enum pcmk__xml_flags mode) static GList * unpack_acl_permission(const xmlNode *xml, GList *acls) { - // ID unset is not possible with schema validation enabled - const char *id = pcmk__s(pcmk__xe_id(xml), "(no ID)"); + const char *id = pcmk__xe_id(xml); const char *type = (const char *) xml->name; + const char *parent_id = pcmk__s(pcmk__xe_id(xml->parent), "without ID"); + const char *parent_type = (const char *) xml->parent->name; + const char *kind_s = pcmk__xe_get(xml, PCMK_XA_KIND); enum pcmk__xml_flags kind = pcmk__xf_none; + if (id == NULL) { + // Not possible with schema validation enabled + pcmk__config_warn("<%s> element in <%s> %s has no " PCMK_XA_ID " " + "attribute", type, parent_type, parent_id); + + // Set a value to use for logging and continue unpacking + id = "without ID"; + } + if (kind_s == NULL) { // Not possible with schema validation enabled - pcmk__warn("Ignoring <%s> element %s with no " PCMK_XA_KIND " " - "attribute", type, id); + pcmk__config_err("Ignoring <%s> element %s (in <%s> %s) with no " + PCMK_XA_KIND " attribute", type, id, parent_type, + parent_id); return acls; } kind = parse_acl_mode(kind_s); if (kind == pcmk__xf_none) { - pcmk__warn("Ignoring <%s> element %s with unknown ACL kind '%s'", type, - id, kind_s); + // Not possible with schema validation enabled + pcmk__config_err("Ignoring <%s> element %s (in <%s> %s) with unknown " + "ACL kind '%s'", type, id, parent_type, parent_id, + kind_s); return acls; } - pcmk__trace("Unpacking <%s> element %s with " PCMK_XA_KIND "='%s'", type, - id, kind_s); + pcmk__trace("Unpacking <%s> element %s (in <%s> %s) with " + PCMK_XA_KIND "='%s'", type, id, parent_type, parent_id, kind_s); return create_acl(xml, acls, kind); }