-
Notifications
You must be signed in to change notification settings - Fork 301
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.
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
HPCC-30111 Look up internal file scope name instead of using a hardcoded value #17684
HPCC-30111 Look up internal file scope name instead of using a hardcoded value #17684
Conversation
https://track.hpccsystems.com/browse/HPCC-30111 |
@@ -795,9 +802,9 @@ class CJwtSecurityManager : implements IDaliLdapConnection, public CBaseSecurity | |||
// Scope hpccinternal::<username> always has full access for their own scope, but | |||
// explicitly denied when attempting to access someone else's | |||
// hpccinternal::<username> scope | |||
if (resourceName && strncmp(resourceName, "hpccinternal::", 14) == 0) | |||
if (strisame(resourceName, hpccInternalScope.c_str())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this will always return false, since the strings are not the same . Consider using something like
if (startsWithIgnoreCase(resourceName, hpccInternalScope.c_str()))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix. Now I'm wondering about my test harness....
{ | ||
if (strisame(&resourceName[14], user.getName())) | ||
if (strisame(&resourceName[hpccInternalScope.length()], user.getName())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above, note how LDAPSecurityManager does it by extracting the name from the filescope
StringBuffer userName;
for (const char * p = &filescope[m_hpccInternalScope.length()]; *p && *p != ':'; p++)//extract scope username
userName.append(*p);
if(strieq(userName.str(), user.getName()))
return SecAccess_Full;
PROGLOG("Access denied to scope %s for user %s", filescope, user.getName());
return SecAccess_None;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which begs the question, how did it work (or did it?) before ? unless the resourceName string always terminates after the username??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The resource name did, in fact, terminate immediately after the username. IIRC from testing the original code, I never saw any "deeper" file scope passed in here. That said, I will revisit and retest once I debug my test setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to find whatever drugs I was on back then and make sure I never take them again.
Testing this morning showed that Russ was right and the code was wrong. It is difficult to see how it ever worked.
I've made the changes Russ suggested and have verified that big jobs that use hpccinternal scopes work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcamper - a couple of comments
|
||
// Grab a copy of the name of the internal file scope | ||
hpccInternalScope = queryDfsXmlBranchName(DXB_Internal); | ||
if (!hpccInternalScope.empty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: since queryDfsXmlBranchName cannot return empty, I think this conditional makes the code less clear (it implies that that queryDfsXmlBranchName(DXB_Internal) might return empty).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, as well as the bonehead no-op within the if() block.
{ | ||
if (strisame(&resourceName[14], user.getName())) | ||
if (strisame(&resourceName[hpccInternalScope.length()], user.getName())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which begs the question, how did it work (or did it?) before ? unless the resourceName string always terminates after the username??
Russ and Jake, please review the changes. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcamper - looks good.
@dcamper Looks good to me |
Previously, "hpccinternal::" was hardcoded. Signed-off-by: Dan S. Camper <[email protected]>
c9836a5
to
e9d94de
Compare
Commits squashed. @ghalliday please merge. Thanks! |
Previously, "hpccinternal::" was hardcoded.
Type of change:
Checklist:
Smoketest:
Testing: