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

wangkx · 2023-07-27T20:38:14Z

The owner ID of a logical file or workunit is based on user's login name which can be case-insensitive. The code for the ID verification should allow the case-insensitive ID as we do in CDaliWorkUnitFactory.getWorkUnitsByOwner().

Change the setFileProtectTree(), getNamedPropTree(), getAttrQueryStr(), and related code to allow case- insensitive owner ID.
Allow case-insensitive owner ID when validating ECL WU access by ownership.
In the IDFUWorkUnitFactory.getWorkUnitsByOwner(), allow case-insensitive owner ID and wildcard match (the same as in CDaliWorkUnitFactory.getWorkUnitsByOwner()).

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

The owner ID of a logical file or workunit is based on user's login name which can be case-insensitive. The code for the ID verification should allow the case-insensitive ID as we do in CDaliWorkUnitFactory.getWorkUnitsByOwner(). 1. Change the setFileProtectTree(), getNamedPropTree(), getAttrQueryStr(), and related code to allow case- insensitive owner ID. 2. Allow case-insensitive owner ID when validating ECL WU access by ownership. 3. In the IDFUWorkUnitFactory.getWorkUnitsByOwner(), allow case-insensitive owner ID and wildcard match (the same as in CDaliWorkUnitFactory.getWorkUnitsByOwner()). Signed-off-by: wangkx <[email protected]>

github-actions · 2023-07-27T20:38:39Z

https://track.hpccsystems.com/browse/HPCC-29970
Jira updated

afishbeck · 2023-08-03T18:57:38Z

dali/dfu/dfuwu.cpp

@@ -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("\"]");


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.

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

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?

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?

@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).

@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.

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

the wildcard question - why should getWorkUnitsByOwner("usera") return results for "notusera" ?

the efficiency issues caused by not performing a standard equality match

wangkx · 2023-08-03T19:31:34Z

@ghalliday please merge.

ghalliday · 2023-08-09T09:22:39Z

It looked good to me. @jakesmith please can you sanity check - e.g., that use of =~ doesn't introduce any major inefficiencies.

jakesmith

@wangkx - a couple of questions.

jakesmith · 2023-08-11T15:44:11Z

dali/dfu/dfuwu.cpp

@@ -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("\"]");


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?

jakesmith · 2023-08-11T15:49:40Z

dali/base/dadfs.cpp

 {
    assertex(key[0]=='@');
-    str.appendf("%s[%s=\"%s\"]",sub,key,name);
+    if (nameCaseInsensitive)
+        str.appendf("%s[%s=?\"%s\"]",sub,key,name);


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.

jakesmith · 2023-08-11T15:56:25Z

dali/dfu/dfuwu.cpp

@@ -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("\"]");


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?

jakesmith

@wangkx - please see new comments.

wangkx · 2023-08-16T12:11:21Z

Need to re-design. Closing for now.

wangkx requested a review from afishbeck July 27, 2023 22:19

afishbeck reviewed Aug 3, 2023

View reviewed changes

afishbeck approved these changes Aug 3, 2023

View reviewed changes

ghalliday requested a review from jakesmith August 9, 2023 09:21

jakesmith reviewed Aug 11, 2023

View reviewed changes

wangkx requested a review from jakesmith August 15, 2023 17:47

jakesmith reviewed Aug 15, 2023

View reviewed changes

wangkx closed this Aug 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wangkx commented Jul 27, 2023

github-actions bot commented Jul 27, 2023

afishbeck Aug 3, 2023

afishbeck Aug 3, 2023

jakesmith Aug 11, 2023

jakesmith Aug 11, 2023

wangkx Aug 12, 2023

jakesmith Aug 15, 2023

jakesmith Aug 15, 2023

wangkx commented Aug 3, 2023

ghalliday commented Aug 9, 2023

jakesmith left a comment

jakesmith Aug 11, 2023

jakesmith Aug 11, 2023

jakesmith Aug 11, 2023

jakesmith left a comment

wangkx commented Aug 16, 2023

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

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

Conversation

wangkx commented Jul 27, 2023

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Jul 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkx commented Aug 3, 2023

ghalliday commented Aug 9, 2023

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

wangkx commented Aug 16, 2023