CNDB-15617: ClienState checks if system before super #2039
Conversation
Checklist before you submit for review
|
driftx
force-pushed
the
CNDB-15617
branch
2 times, most recently
from
440ec9f to
a63caea
Compare
| return !isSuperIgnoreAnonymousUser() && !isSystem(); | ||
| return !isSuper() && !isSystem(); | ||
| return !isSystem() && !isSuperIgnoreAnonymousUser(); | ||
| return !isSystem() && !isSuper(); |
There was a problem hiding this comment.
A comment here may be nice to raise awareness that isSystem is "cheap" while isSuper and isSuperIgnoreAnonymousUser may/will trigger possibly expensive database calls (if not cached) or be called before auth tables are fully initialized.
Though the fact that auth tables are not initialized makes me wonder - is that a situation arising only with CNDB or also with C*, regardless if the auth user is SYSTEM_USER or not? Does it imply any precondition has not been satisfied?
This leads to ask how this affects HCD or even OSS C*?
There was a problem hiding this comment.
I added a comment about how system users should bypass all guardrails and permissions, which was the real issue here (though avoiding the possibly more expensive query is nice to have.) In 5.0, ENABLE_GUARDRAILS_FOR_ANONYMOUS_USER defaults to true, so external calls by system users would get caught there a) if auth wasn't ready and b) because isSystem wouldn't recognize them.
So it requires external calls by system users to be an issue, which is a situation CNDB satisfies but I can't think of others off the top of my head. I don't think it impacts HCD or OSS until that situation occurs.
There was a problem hiding this comment.
auth tables are not initialized
I don't know without testing it again but I think accessing DatabaseDescriptor.getAuthenticator().requireAuthentication() in the isSuper call was problematic for CNDB specifically at that point. It may have been fixable some other way there, but we'd still have to fix isSystem.
| public boolean isSystem() | ||
| { | ||
| return isInternal; | ||
| return isInternal || (user != null && user.isSystem()); |
There was a problem hiding this comment.
Add an explanatory comment please? I tend to find the code in this class simple and terse which can be confusing because the code paths that use these methods are not always to clear to understand. I don't know if others feel the same though.
There was a problem hiding this comment.
I added javadoc saying this returns true for both internal and external calls, does that help?
There was a problem hiding this comment.
The change looks good.
Suggested addition of some code comments to help document/explain things.
Also left a question if the auth tables being 'not ready' is of any concern and if there is any impact for HCD/OSS C* in addition to CNDB.
Unless there is reason to believe CNDB is not starting C* correctly somehow (e.g. waiting for auth tables to be init'd) I don't think there is a reason to hold the changes here. This seems like a good defensive change even if that is the case.
|
❌ Build ds-cassandra-pr-gate/PR-2039 rejected by Butler2 regressions found Found 2 new test failures
Found 5 known test failures |
3 participants
What is the issue
ClientState.isSystem()only checks theisInternalfield, which is false for external client states created viaforExternalCalls(user). This causes the method to return false even when the authenticated user isAuthenticatedUser.SYSTEM_USER.This leads to incorrect behavior in
isOrdinaryUser(), which callsisSuper()forSYSTEM_USER, triggering expensive database queries viaRoles.hasSuperuserStatus(). During system initialization when auth tables aren't fully ready, this causesNullPointerExceptionbecauseCassandraRoleManager.loadRoleStatementmay not be initialized yet.What does this PR fix and why was it fixed
Change
isSystem()to check both the internal flag and the user identity, changeisOrdinaryUser()to check system users before super users.