Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HPCC-29970 Check case-insensitive owner ID in LF/WU #17624

Closed
wants to merge 1 commit into from
Closed

HPCC-29970 Check case-insensitive owner ID in LF/WU #17624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 24 additions & 21 deletions dali/base/dadfs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,19 +138,22 @@ static StringBuffer &normalizeFormat(StringBuffer &in)
return in;
}

static StringBuffer &getAttrQueryStr(StringBuffer &str,const char *sub,const char *key,const char *name)
static StringBuffer &getAttrQueryStr(StringBuffer &str,const char *sub,const char *key,const char *name, bool nameCaseInsensitive)
{
assertex(key[0]=='@');
str.appendf("%s[%s=\"%s\"]",sub,key,name);
if (nameCaseInsensitive)
str.appendf("%s[%s=?\"%s\"]",sub,key,name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as long as these are not top-level properties, and the only one nameCaseInsnsitive=true is used for is for Protect/@name, then the loss of efficiency this implies is okay I think.

else
str.appendf("%s[%s=\"%s\"]",sub,key,name);
return str;
}

static IPropertyTree *getNamedPropTree(const IPropertyTree *parent, const char *sub, const char *key, const char *name, bool preload)
static IPropertyTree *getNamedPropTree(const IPropertyTree *parent, const char *sub, const char *key, const char *name, bool preload, bool nameCaseInsensitive)
{ // no create
if (!parent)
return NULL;
StringBuffer query;
getAttrQueryStr(query,sub,key,name);
getAttrQueryStr(query,sub,key,name,nameCaseInsensitive);
if (preload)
return parent->getBranch(query.str());
return parent->getPropTree(query.str());
Expand Down Expand Up @@ -417,7 +420,7 @@ void ensureFileScope(const CDfsLogicalFileName &dlfn,unsigned timeout)
query.append(e-s,s);
else
query.append(s);
nr = getNamedPropTree(r,queryDfsXmlBranchName(DXB_Scope),"@name",query.trim().toLowerCase().str(),false);
nr = getNamedPropTree(r,queryDfsXmlBranchName(DXB_Scope),"@name",query.trim().toLowerCase().str(),false,false);
if (!nr)
nr = addNamedPropTree(r,queryDfsXmlBranchName(DXB_Scope),"@name",query.str());
r->Release();
Expand Down Expand Up @@ -754,12 +757,12 @@ class CScopeConnectLock
StringBuffer tail;
dlfn.getTail(tail);
StringBuffer query;
getAttrQueryStr(query,queryDfsXmlBranchName(DXB_File),"@name",tail.str());
getAttrQueryStr(query,queryDfsXmlBranchName(DXB_File),"@name",tail.str(),false);
IPropertyTree *froot = sroot->queryPropTree(query.str());
bkind = DXB_File;
if (!froot) {
// check for super file
getAttrQueryStr(query.clear(),queryDfsXmlBranchName(DXB_SuperFile),"@name",tail.str());
getAttrQueryStr(query.clear(),queryDfsXmlBranchName(DXB_SuperFile),"@name",tail.str(),false);
froot = sroot->queryPropTree(query.str());
if (froot)
bkind = DXB_SuperFile;
Expand Down Expand Up @@ -2701,7 +2704,7 @@ static bool setFileProtectTree(IPropertyTree &p,const char *owner, bool protect)
dt.setNow();
if (owner&&*owner)
{
Owned<IPropertyTree> t = getNamedPropTree(&p, "Protect", "@name", owner, false);
Owned<IPropertyTree> t = getNamedPropTree(&p, "Protect", "@name", owner, false, true);
if (t)
{
if (protect)
Expand Down Expand Up @@ -3216,7 +3219,7 @@ class CDistributedFileBase : implements INTERFACE, public CInterface
CFileSuperOwnerLock attrLock;
if (0 == proplockcount)
verifyex(attrLock.init(logicalName, conn, defaultTimeout, "CDistributedFile::linkSuperOwner"));
Owned<IPropertyTree> t = getNamedPropTree(root,"SuperOwner","@name",superfile,false);
Owned<IPropertyTree> t = getNamedPropTree(root,"SuperOwner","@name",superfile,false,false);
if (t && !link)
root->removeTree(t);
else if (!t && link)
Expand Down Expand Up @@ -7654,7 +7657,7 @@ class CNamedGroupStore: implements INamedGroupStore, public CInterface
return nullptr;
}
}
Owned<IPropertyTree> groupTree = getNamedPropTree(conn->queryRoot(), "Group", "@name", gname.str(), true);
Owned<IPropertyTree> groupTree = getNamedPropTree(conn->queryRoot(), "Group", "@name", gname.str(), true, false);
if (!groupTree || !loadGroup(groupTree, epa, &type, &groupdir))
return nullptr;
}
Expand Down Expand Up @@ -7892,7 +7895,7 @@ class CNamedGroupStore: implements INamedGroupStore, public CInterface
prop.appendf("Group[@name=\"%s\"]",name.str());
CConnectLock connlock("CNamedGroup::add", SDS_GROUPSTORE_ROOT, true, false, false, defaultTimeout);

Owned<IPropertyTree> groupTree = getNamedPropTree(connlock.conn->queryRoot(), "Group", "@name", name, true);
Owned<IPropertyTree> groupTree = getNamedPropTree(connlock.conn->queryRoot(), "Group", "@name", name, true, false);
if (!groupTree)
return 0;
SocketEndpointArray epa;
Expand Down Expand Up @@ -8981,9 +8984,9 @@ void CDistributedFileDirectory::addEntry(CDfsLogicalFileName &dlfn,IPropertyTree
IPropertyTree* sroot = sconnlock.conn()->queryRoot();
StringBuffer tail;
dlfn.getTail(tail);
IPropertyTree *prev = getNamedPropTree(sroot,superfile?queryDfsXmlBranchName(DXB_SuperFile):queryDfsXmlBranchName(DXB_File),"@name",tail.str(),false);
IPropertyTree *prev = getNamedPropTree(sroot,superfile?queryDfsXmlBranchName(DXB_SuperFile):queryDfsXmlBranchName(DXB_File),"@name",tail.str(),false,false);
if (!prev) // check super/file doesn't exist
prev = getNamedPropTree(sroot,superfile?queryDfsXmlBranchName(DXB_File):queryDfsXmlBranchName(DXB_SuperFile),"@name",tail.str(),false);
prev = getNamedPropTree(sroot,superfile?queryDfsXmlBranchName(DXB_File):queryDfsXmlBranchName(DXB_SuperFile),"@name",tail.str(),false,false);
if (prev!=nullptr)
{
prev->Release();
Expand Down Expand Up @@ -11005,7 +11008,7 @@ class CDaliDFSServer: public Thread, public CTransactionLogTracker, implements I
CScopeConnectLock sconnlock("setFileAccessed", dlfn, false, false, false, defaultTimeout);
IPropertyTree* sroot = sconnlock.conn()?sconnlock.conn()->queryRoot():NULL;
dlfn.getTail(tail);
Owned<IPropertyTree> tree = getNamedPropTree(sroot,queryDfsXmlBranchName(DXB_File),"@name",tail.str(),false);
Owned<IPropertyTree> tree = getNamedPropTree(sroot,queryDfsXmlBranchName(DXB_File),"@name",tail.str(),false,false);
if (tree) {
StringBuffer str;
tree->setProp("@accessed",dt.getString(str).str());
Expand Down Expand Up @@ -11036,9 +11039,9 @@ class CDaliDFSServer: public Thread, public CTransactionLogTracker, implements I
CScopeConnectLock sconnlock("setFileProtect", dlfn, false, false, false, defaultTimeout);
IPropertyTree* sroot = sconnlock.conn()?sconnlock.conn()->queryRoot():NULL;
dlfn.getTail(tail);
Owned<IPropertyTree> tree = getNamedPropTree(sroot,queryDfsXmlBranchName(DXB_File),"@name",tail.str(),false);
Owned<IPropertyTree> tree = getNamedPropTree(sroot,queryDfsXmlBranchName(DXB_File),"@name",tail.str(),false,false);
if (!tree)
tree.setown(getNamedPropTree(sroot,queryDfsXmlBranchName(DXB_SuperFile),"@name",tail.str(),false));
tree.setown(getNamedPropTree(sroot,queryDfsXmlBranchName(DXB_SuperFile),"@name",tail.str(),false,false));
if (tree) {
IPropertyTree *pt = tree->queryPropTree("Attr");
if (pt)
Expand Down Expand Up @@ -11113,7 +11116,7 @@ class CDaliDFSServer: public Thread, public CTransactionLogTracker, implements I
logicalname->getTail(tail);
if (version >= 2)
{
Owned<IPropertyTree> tree = getNamedPropTree(sroot,queryDfsXmlBranchName(DXB_File),"@name",tail.str(),false);
Owned<IPropertyTree> tree = getNamedPropTree(sroot,queryDfsXmlBranchName(DXB_File),"@name",tail.str(),false,false);
if (tree)
{
#ifdef _CONTAINERIZED
Expand All @@ -11138,7 +11141,7 @@ class CDaliDFSServer: public Thread, public CTransactionLogTracker, implements I
}
else
{
tree.setown(getNamedPropTree(sroot,queryDfsXmlBranchName(DXB_SuperFile),"@name",tail.str(),false));
tree.setown(getNamedPropTree(sroot,queryDfsXmlBranchName(DXB_SuperFile),"@name",tail.str(),false,false));
if (tree)
{
mb.append((int)2); // 2 == super
Expand All @@ -11149,7 +11152,7 @@ class CDaliDFSServer: public Thread, public CTransactionLogTracker, implements I
}
else
{
Owned<IPropertyTree> tree = getNamedPropTree(sroot,queryDfsXmlBranchName(DXB_File),"@name",tail.str(),false);
Owned<IPropertyTree> tree = getNamedPropTree(sroot,queryDfsXmlBranchName(DXB_File),"@name",tail.str(),false,false);
if (tree)
{
if (version == MDFS_GET_FILE_TREE_V2)
Expand Down Expand Up @@ -11187,7 +11190,7 @@ class CDaliDFSServer: public Thread, public CTransactionLogTracker, implements I
}
else
{
tree.setown(getNamedPropTree(sroot,queryDfsXmlBranchName(DXB_SuperFile),"@name",tail.str(),false));
tree.setown(getNamedPropTree(sroot,queryDfsXmlBranchName(DXB_SuperFile),"@name",tail.str(),false,false));
if (tree)
{
tree->serialize(mb);
Expand Down Expand Up @@ -11223,7 +11226,7 @@ class CDaliDFSServer: public Thread, public CTransactionLogTracker, implements I
transactionLog.log("%s", trc.str());
byte ok;
CConnectLock connlock("getGroupTree",SDS_GROUPSTORE_ROOT,false,false,false,defaultTimeout);
Owned<IPropertyTree> pt = getNamedPropTree(connlock.conn->queryRoot(),"Group","@name",gname.get(),true);
Owned<IPropertyTree> pt = getNamedPropTree(connlock.conn->queryRoot(),"Group","@name",gname.get(),true,false);
if (pt) {
ok = 1;
mb.append(ok);
Expand Down
2 changes: 1 addition & 1 deletion dali/dfu/dfuwu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3287,7 +3287,7 @@ class CDFUWorkUnitFactory : implements IDFUWorkUnitFactory, implements ISDSSubsc
{
StringBuffer path("*");
if (owner && *owner)
path.append("[@submitID=\"").append(owner).append("\"]");
path.append("[@submitID=?~\"").append(owner).append("\"]");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the '~' to specify a wild match? If so, that MIGHT not be needed anymore (ptree might always support wild card characters?). but I guess it doesn't hurt.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said doesn't hurt and makes the wildcard more explicit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the implication is that wildcard is desired here? (it would not have done so before with a regular equality qualifier)

I would not expect getWorkUnitsByOwner("usera") to also return dfu workunits owned by "notusera" ..

I do notice it's already this way for ecl workunits in it's implementation of getWorkUnitsByOwner, but should it be?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, using anything but a regular equality is lot more inefficient, because jptree optimizes lookups of sibling via qualifiers on attributes with straight equalities (by auto building HT's on the fly), so using "=?" (or =?~") will result in a linear scan of all dfu workunits instead.

The normal approach to case-insensitivity employed elsewhere is to store it lower case it when set, and looked up.
Perhaps the original cased submitID is needed here(?), but we could add an internal attribute for this purpose instead and thus avoid "=?".
Either way, that won't help with pre-existing workunits that don't have the lower-case @submitID (but could be 'fixed' automatically via other changes).

@wangkx - first question is - does submitID need to preserve case when it is set?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jakesmith I have 3 concerns for not preserving case. 1. confuse a user ('who changed my username?' or 'the submitID was not in lower case on previous ECLWatch page'). 2. how to handle old WUs which contain case sensitive submitID? 3. break some existing logic (where? I do not know).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wangkx - this is quite a significant performance issue.

  • @submitID doesn't need to change.
  • A new internal attribute can be added (lower-cased) explicitly for the purpose of optimized caseless searching.
  • Old WUs can be scanned for the new missing attribute at startup time (it would be best to do in Dali itself), and the new attribute added.
  • Code for searching attributed can use regular hard equality matching, which will be an order of magnitude faster than the linear search that =? would involve.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this one was marked as resolved.
There are 2 relates issues here:

  1. the wildcard question - why should getWorkUnitsByOwner("usera") return results for "notusera" ?
  2. the efficiency issues caused by not performing a standard equality match

return getWorkUnitsByXPath(path.str());
}
IConstDFUWorkUnitIterator * getWorkUnitsByState(DFUstate state)
Expand Down
4 changes: 2 additions & 2 deletions esp/services/ws_workunits/ws_workunitsHelpers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ const char* zipFolder = "tempzipfiles" PATHSEPSTR;

SecAccessFlags chooseWuAccessFlagsByOwnership(const char *user, const char *owner, SecAccessFlags accessOwn, SecAccessFlags accessOthers)
{
return (isEmpty(owner) || (user && streq(user, owner))) ? accessOwn : accessOthers;
return (isEmpty(owner) || (user && strieq(user, owner))) ? accessOwn : accessOthers;
}

SecAccessFlags chooseWuAccessFlagsByOwnership(const char *user, IConstWorkUnitInfo& cw, SecAccessFlags accessOwn, SecAccessFlags accessOthers)
Expand All @@ -59,7 +59,7 @@ SecAccessFlags chooseWuAccessFlagsByOwnership(const char *user, IConstWorkUnitIn

const char *getWuAccessType(const char *owner, const char *user)
{
return (isEmpty(owner) || (user && streq(user, owner))) ? OWN_WU_ACCESS : OTHERS_WU_ACCESS;
return (isEmpty(owner) || (user && strieq(user, owner))) ? OWN_WU_ACCESS : OTHERS_WU_ACCESS;
}

const char *getWuAccessType(IConstWorkUnit& cw, const char *user)
Expand Down
Loading