ACL refactors: Clarify and deprecate xml_acl_filtered_copy()#4132
Open
nrwahl2 wants to merge 16 commits into
Open
ACL refactors: Clarify and deprecate xml_acl_filtered_copy()#4132nrwahl2 wants to merge 16 commits into
nrwahl2 wants to merge 16 commits into
Conversation
And make it static. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This is similar to pcmk__xml_tree_foreach(), except for the following: * It removes a node and its subtree if the function returns true. pcmk__xml_tree_foreach() would have a use-after-free if its function freed its xmlNode * argument. (This has not been observed with any existing calls.) * pcmk__xml_tree_foreach_remove() currently offers no way to stop iterating. The function's return value instead tells us whether to remove a node. Obviously we don't recurse down a freed node's subtree, however. * The function does not currently accept user data. A user data argument may be added later if needed. * pcmk__xml_tree_foreach_remove() returns void. We can later return a boolean or integer value if we need to know whether any nodes were removed. Nothing uses this yet. Also correct the Doxygen of pcmk__xml_tree_foreach() (fn is an [in] parameter) and add a note that fn may not remove its argument. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
nrwahl2
force-pushed
the
nrwahl2-acls_first
branch
from
d8c9d0b to
b08f260
Compare
Contributor
Author
|
Previous fix put the Coverity annotation on the wrong line. Still running locally to be sure, but the new push should be correct. |
clumens
reviewed
Jun 18, 2026
| * \return \c true if \c pcmk__xf_deleted is set for \p attr, or \c false | ||
| * otherwise | ||
| * | ||
| * \note This is compatible with \c pcmk__xe_remove_matching_attrs(). |
Contributor
There was a problem hiding this comment.
Just a note... I really like this comment, and I wish I'd been doing something like this when I previously added list traversal functions.
Contributor
Author
There was a problem hiding this comment.
Good to know! I was on the fence about whether this was gratuitous.
| readable_children = true; | ||
| } | ||
| } else if (pcmk__is_set(nodepriv->flags, pcmk__xf_acl_deny)) { | ||
| in_readable_context = false; |
Contributor
There was a problem hiding this comment.
I see that in_readable_context gets used after the recursive call to filter_unreachable_nodes. Did you intend it to be an out parameter instead?
Contributor
Author
There was a problem hiding this comment.
No, I don't think so. Each child node inherits its parent's in_readable_context via the recursive call. Then the value may be overridden within the child's call, if an ACL applies directly to the child. That's what these pcmk__xf_acl_{read,deny} blocks are doing -- checking whether there's an ACL that applies directly to the current (child) node.
But a child's value for in_readable_context shouldn't affect the parent's value.
And change its name to pcmk__check_creation_acls(), since it's checking rather than applying ACLs. Use pcmk__xml_tree_foreach_remove() to "drive" the recursion, instead of building it into pcmk__check_creation_acls(). Functionize the main logic into the helper check_creation_disallowed(). Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
If xml->doc or xml->doc->_private were NULL, then we would not have called this function. See pcmk__xml_doc_all_flags_set(). Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Note to reviewer: this code is expected to be removed a few commits from now. This commit was part of my process of trying to understand the filtering logic. It may make review easier, or it may make it more tedious. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Note to reviewer: this code is expected to be removed a few commits from now. This commit was part of my process of trying to understand the filtering logic. It may make review easier, or it may make it more tedious. Use pcmk__xpath_foreach_result(). Also drop a trace message, for two reasons: * We don't have easy access to num_results anymore. (We'd have to get it via a counter variable or something.) * The message tells us how many nodes were filtered by a given XPath, but not which nodes. So it doesn't seem especially useful. If we want tracing for ACL filtering (which seems like a reasonable thing to want), then we should make it more granular so that it's informative. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
There's no reason to filter all the children if we can already tell that we're going to deny the user access to the entire document. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This is less for efficiency and more for understandability, before we make a larger change to this code. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This is a substantial refactor. Most of the details are in the comments.
It took me days to understand exactly what xml_acl_filtered_copy() was
doing, particularly in the calls to purge_xml_attributes(). For a while
I was pretty sure that we could be exposing "denied" children if an
ancestor had the pcmk__xf_acl_read or pcmk__xf_acl_write flag set. I'm
no longer confident that was the case, although it may have been.
The whole process was convoluted. We unpacked all of a user's ACLs and
applied them to matching nodes throughout the document. Then we went
through the unpacked list of a user's ACLs (docpriv->acls) and found the
ones with "deny" permissions and non-NULL xpath. (By the way, I believe
all ACLs should have non-NULL xpath by that point.)
Then for each of those "deny" ACLs, we filtered the document based on
it. We ran an XPath query to find all the nodes in the document that
matched the "deny" ACL, and we called purge_xml_attributes() on each
such node. A return value of true indicated that the current node either
was readable or had at least one readable descendant.
* If the current node had the pcmk__xf_acl_read or pcmk__xf_acl_write
flag set (is_mode_allowed()), then it was readable and we returned
true immediately. Some read or write ACL had matched that node during
the apply stage. Of course this could never be true for the top-level
call, because the top-level call was always for some node that a
"deny" ACL had matched.
* Otherwise, the current node was unreadable. We called
purge_xml_attributes() on all children.
- If any children returned true, then we had at least one readable
descendant. We removed all attributes except for ID and then
returned true.
- Otherwise, we had no readable descendants. Since the current node
was also unreadable, we removed the current node's subtree and
returned false.
As mentioned, this was convoluted. The key observation is that before we
did any of that, we had already applied all the ACLs throughout the
document. So all we have to do is recurse from the top down, determine
which nodes are readable for the current ACL user, and filter out the
rest. The comments explain how this works.
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To replace xml_acl_filtered_copy(). Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Perform a CIB query as a given user if you want to filter CIB XML as that user. This function shouldn't have been used for arbitrary (non-CIB) XML anyway. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
nrwahl2
force-pushed
the
nrwahl2-acls_first
branch
from
b08f260 to
1cfc29d
Compare
Contributor
Author
|
Updated to address review (including the commit message mistake), and to replace |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the next batch from #4013.