Skip to content

Commit 4b0b96f

Browse files
esindrilesindril
committed
MGM: Avoid listing ACL duplicates when space level ACLs are configured and
these match already existing ACLs on entries. Add tests for this situation. Fixes EOS-6529
1 parent 1ee2ed3 commit 4b0b96f

File tree

2 files changed

+119
-64
lines changed

2 files changed

+119
-64
lines changed

‎mgm/XrdMgmOfs/Attr.cc‎

Lines changed: 106 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -493,8 +493,8 @@ XrdMgmOfs::_attr_rem(const char* path, XrdOucErrInfo& error,
493493
eos::IContainerMD::XAttrMap attr_map = cmd->getAttributes();
494494
Acl acl(attr_map, vid);
495495

496-
if (vid.token || (!cmd->access(vid.uid, vid.gid, X_OK | W_OK) &&
497-
!acl.AllowXAttrUpdate(skey, vid))) {
496+
if (vid.token || (!cmd->access(vid.uid, vid.gid, X_OK | W_OK) &&
497+
!acl.AllowXAttrUpdate(skey, vid))) {
498498
errno = EPERM;
499499
} else {
500500
if (!cmd->hasAttribute(skey)) {
@@ -527,108 +527,151 @@ XrdMgmOfs::_attr_rem(const char* path, XrdOucErrInfo& error,
527527
}
528528

529529
//----------------------------------------------------------------------------
530-
//! List Attributes high-level function merging space and namespace attributes
530+
// List attributes high-level function merging space and namespace attributes
531531
//----------------------------------------------------------------------------
532-
533532
void
534-
XrdMgmOfs::mergeSpaceAttributes(eos::IContainerMD::XAttrMap& out, bool prefix, bool existing){
533+
XrdMgmOfs::mergeSpaceAttributes(eos::IContainerMD::XAttrMap& out, bool prefix,
534+
bool existing)
535+
{
535536
std::string space = "default";
537+
536538
if (out.count("sys.forced.space")) {
537539
space = out["sys.forced.space"];
538540
}
539-
std::map<std::string,std::string> attr;
541+
542+
std::map<std::string, std::string> attr;
543+
540544
if (gOFS->mSpaceAttributes.count(space)) {
541-
// retrieve space arguments
542-
std::unique_lock<std::mutex> lock(gOFS->mSpaceAttributesMutex);
543-
attr = gOFS->mSpaceAttributes[space];
544-
}
545-
// loop over arguments
546-
for ( auto x:attr ) {
547-
if (x.first == "sys.forced.space") {
548-
// we ignore this
545+
// retrieve space arguments
546+
std::unique_lock<std::mutex> lock(gOFS->mSpaceAttributesMutex);
547+
attr = gOFS->mSpaceAttributes[space];
548+
}
549+
550+
// loop over arguments
551+
for (const auto& x : attr) {
552+
if (x.first == "sys.forced.space") {
553+
// we ignore this
554+
continue;
555+
} else {
556+
if (existing && !out.count(x.first)) {
557+
// merge only existing attributes
558+
continue;
559+
}
560+
561+
std::string inkey = x.first;
562+
std::string outkey = (prefix ? (std::string("sys.space.") + x.first) : x.first);
563+
564+
if (x.first == "sys.acl") {
565+
// Special meaning of the first char:
566+
// > append acl to the existing ones
567+
// < prepend acl to the existing ones
568+
// | add only if acls not set at all
569+
// none of the above means overwrite existring acls
570+
char op = '\0';
571+
572+
if (!x.second.empty()) {
573+
op = x.second[0];
574+
}
575+
576+
std::string old_acls = out["sys.acl"];
577+
std::string space_acls = x.second;
578+
579+
if ((op == '>') || (op == '<') || (op == '|')) {
580+
space_acls = space_acls.erase(0, 1);
581+
}
582+
583+
// ACL handling
584+
if (((op != '>') && (op != '<') && (op != '|')) || // Full overwrite
585+
((op == '|') && old_acls.empty())) { // Overwrite if empty
586+
out[outkey] = space_acls;
587+
} else {
588+
// If existing acls already include the space acls then
589+
// remove them to avoid duplicates.
590+
auto pos = old_acls.find(space_acls);
591+
592+
if ((pos != std::string::npos) && (op != '|')) {
593+
auto del_pos = pos;
594+
auto del_len = space_acls.length();
595+
596+
// Either delete the command before or the comma after
597+
if (del_pos && (old_acls[del_pos - 1] == ',')) {
598+
--del_pos;
599+
++del_len;
600+
} else if ((del_pos + del_len < old_acls.length()) &&
601+
(old_acls[del_pos + del_len] == ',')) {
602+
++del_len;
603+
}
604+
605+
old_acls = old_acls.erase(del_pos, del_len);
606+
}
607+
608+
if (op == '>') { // Append rule
609+
out[outkey] = old_acls + std::string(",") + space_acls;
610+
} else if (op == '<') { // Prepend rule
611+
out[outkey] = space_acls + std::string(",") + old_acls;
612+
}
613+
614+
if (op == '|') {
615+
out[outkey] = old_acls;
616+
}
617+
}
549618
} else {
550-
if (existing && !out.count(x.first)) {
551-
// merge only existing attributes
552-
continue;
553-
}
554-
std::string inkey=x.first;
555-
std::string outkey=prefix?(std::string("sys.space.") + x.first): x.first;
556-
if (x.first == "sys.acl") {
557-
// ACL handling
558-
if (x.second.substr(0,1) == ">") {
559-
if (out["sys.acl"].length()) {
560-
// append rule
561-
out[outkey] = out["sys.acl"] + std::string(",") + x.second.substr(1);
562-
} else {
563-
out[outkey] = x.second.substr(1);
564-
}
565-
} else if (x.second.substr(0,1) == "<") {
566-
if (out["sys.acl"].length()) {
567-
// prepend rule
568-
out[outkey] = x.second.substr(1) + std::string(",") + out["sys.acl"];
569-
} else {
570-
out[outkey] = x.second.substr(1);
571-
}
572-
} else if (x.second.substr(0,1) == "|") {
573-
if (!out["sys.acl"].length()) {
574-
// if not set rule
575-
out[outkey] = x.second.substr(1);
576-
}
577-
} else {
578-
// overwrite rule
579-
out[outkey] = x.second;
580-
}
581-
} else {
582-
// normal attribute handling
583-
if (x.second.substr(0,1) == "|") {
584-
// if not set rule
585-
if (!out[x.first].length()) {
586-
out[outkey] = x.second.substr(1);
587-
}
588-
} else {
589-
// overwrite rule
590-
out[outkey] = x.second;
591-
}
592-
}
619+
// Normal attribute handling
620+
if (x.second.substr(0, 1) == "|") {
621+
// if not set rule
622+
if (!out[x.first].length()) {
623+
out[outkey] = x.second.substr(1);
624+
}
625+
} else {
626+
// overwrite rule
627+
out[outkey] = x.second;
628+
}
593629
}
594630
}
631+
}
595632
}
596633

597634

598635
void
599636
XrdMgmOfs::listAttributes(eos::IView* view, eos::IContainerMD* target,
600-
eos::IContainerMD::XAttrMap& out, bool prefixLinks){
637+
eos::IContainerMD::XAttrMap& out, bool prefixLinks)
638+
{
601639
eos::listAttributes(view, target, out, prefixLinks);
602640
mergeSpaceAttributes(out);
603641
}
604642

605643
void
606644
XrdMgmOfs::listAttributes(eos::IView* view, eos::IFileMD* target,
607-
eos::IContainerMD::XAttrMap& out, bool prefixLinks){
645+
eos::IContainerMD::XAttrMap& out, bool prefixLinks)
646+
{
608647
eos::listAttributes(view, target, out, prefixLinks);
609648
mergeSpaceAttributes(out);
610649
}
611650

612651
void
613652
XrdMgmOfs::listAttributes(eos::IView* view, eos::FileOrContainerMD target,
614-
eos::IContainerMD::XAttrMap& out, bool prefixLinks){
653+
eos::IContainerMD::XAttrMap& out, bool prefixLinks)
654+
{
615655
eos::listAttributes(view, target, out, prefixLinks);
616656
mergeSpaceAttributes(out);
617657
}
618658

619659
template<typename T>
620660
bool XrdMgmOfs::getAttribute(eos::IView* view, T& md, std::string key,
621-
std::string& rvalue)
661+
std::string& rvalue)
622662
{
623663
auto result = eos::getAttribute(view, md, key, rvalue);
624664
eos::IContainerMD::XAttrMap attr;
665+
625666
if (!result) {
626-
attr[key]="";
667+
attr[key] = "";
627668
} else {
628669
attr[key] = rvalue;
629670
}
630-
mergeSpaceAttributes(attr,false,true);
671+
672+
mergeSpaceAttributes(attr, false, true);
631673
rvalue = attr[key];
674+
632675
if (!result) {
633676
return attr[key].length();
634677
} else {

‎test/eos-instance-test‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1356,7 +1356,19 @@ runtest "### SpaceACL" unix 0 "" eos space config default "space.attr.sys.acl=\>
13561356
runtest "### SpaceACL" unix 0 "" eos attr ls "/eos/$EOS_TEST_INSTANCE/test/acl/nobody_src/"
13571357
runtest "### SpaceACL" unix 0 "" eos attr ls "/eos/$EOS_TEST_INSTANCE/test/acl/nobody_src/ | grep -F 'u:65534:rw,u:2:rwx!d,u:1000:rwx'"
13581358
runtest "### SpaceACL" unix 0 "" eos space config rm default "space.attr.sys.acl"
1359-
runtest "### SpaceACL" unix 1 "" eos attr ls "/eos/$EOS_TEST_INSTANCE/test/acl/nobody_src/ | grep u:1000:rwx"
1359+
# Test that if directory already has the space acl then we don't see duplicates
1360+
runtest "### SpaceACL" sss 0 "" eos acl u:1000=rx "/eos/$EOS_TEST_INSTANCE/test/acl/nobody_src/"
1361+
runtest "### SpaceACL" unix 0 "" eos attr ls "/eos/$EOS_TEST_INSTANCE/test/acl/nobody_src/ | grep -F 'u:65534:rw,u:2:rwx!d,u:1000:rx'"
1362+
runtest "### SpaceACL" unix 0 "" eos space config default space.attr.sys.acl=u:1000:rx
1363+
runtest "### SpaceACL" unix 0 "" eos attr ls "/eos/$EOS_TEST_INSTANCE/test/acl/nobody_src/ | grep u:1000:rx | grep -v u:65534:rw"
1364+
runtest "### SpaceACL" unix 0 "" eos space config default "space.attr.sys.acl=\|u:1000:rx"
1365+
runtest "### SpaceACL" unix 0 "" eos attr ls "/eos/$EOS_TEST_INSTANCE/test/acl/nobody_src/ | grep u:1000:rx | grep u:65534:rw"
1366+
runtest "### SpaceACL" unix 0 "" eos space config default "space.attr.sys.acl=\<u:1000:rx"
1367+
runtest "### SpaceACL" unix 0 "" eos attr ls "/eos/$EOS_TEST_INSTANCE/test/acl/nobody_src/ | grep -F 'u:1000:rx,u:65534:rw,u:2:rwx!d'"
1368+
runtest "### SpaceACL" unix 0 "" eos space config default "space.attr.sys.acl=\>u:1000:rx"
1369+
runtest "### SpaceACL" unix 0 "" eos attr ls "/eos/$EOS_TEST_INSTANCE/test/acl/nobody_src/ | grep -F 'u:65534:rw,u:2:rwx!d,u:1000:rx'"
1370+
runtest "### SpaceACL" unix 0 "" eos space config rm default "space.attr.sys.acl"
1371+
runtest "### SpaceACL" unix 0 "" eos attr ls "/eos/$EOS_TEST_INSTANCE/test/acl/nobody_src/ | grep u:1000:rx"
13601372
runtest "### Rmdir " unix 0 "" eos rm -rf "/eos/$EOS_TEST_INSTANCE/test/acl"
13611373
# Test concurrent ACL recursive modifications
13621374
runtest "### Mkdir " unix 0 "" eos mkdir "/eos/$EOS_TEST_INSTANCE/test/acl_concurrent"

0 commit comments

Comments
 (0)