enh: privilege parse and check create stream#34143
Conversation
Summary of ChangesHello @kailixu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on strengthening the security and correctness of privilege management within the system. It introduces more robust authorization checks for Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR enhances privilege parsing and checking for CREATE STREAM operations by adding conditional logic for optional database targets and improving grant statement serialization.
Key Changes:
- Added conditional privilege checks for optional
targetDbNamein stream creation, preventing unnecessary authorization when target database is not specified - Added privilege check for trigger table database access (PRIV_DB_USE)
- Improved grant statement serialization to support multiple operation types (privileges vs roles) with proper conditional handling
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| source/libs/parser/src/parAuthenticator.c | Added PRIV_DB_USE check for trigger table database and conditional targetDbName privilege checks |
| source/libs/parser/src/parAstParser.c | Reorganized privilege reservation calls and added conditional logic for targetDbName |
| source/libs/nodes/src/nodesCodeFuncs.c | Enhanced grant statement serialization with tabName and optrType support, improved privilege parsing with strsplit |
Comments suppressed due to low confidence (1)
source/libs/nodes/src/nodesCodeFuncs.c:9362
- The changes to grant statement serialization logic (adding tabName and optrType fields, changing privilege parsing) lack test coverage. Consider adding tests to verify that serialization and deserialization work correctly for all cases, including when tabName is empty and for different optrType values (TSDB_ALTER_ROLE_PRIVILEGES and TSDB_ALTER_ROLE_ROLE).
static int32_t grantStmtToJson(const void* pObj, SJson* pJson) {
const SGrantStmt* pNode = (const SGrantStmt*)pObj;
int32_t code = tjsonAddStringToObject(pJson, jkGrantStmtUserName, pNode->principal);
if (TSDB_CODE_SUCCESS == code) {
code = tjsonAddStringToObject(pJson, jkGrantStmtObjName, pNode->objName);
}
if (TSDB_CODE_SUCCESS == code) {
if (pNode->tabName[0] != '\0') {
code = tjsonAddStringToObject(pJson, jkNameTableName, pNode->tabName);
}
}
if (TSDB_CODE_SUCCESS == code) {
code = tjsonAddIntegerToObject(pJson, "optrType", pNode->optrType);
}
if (TSDB_CODE_SUCCESS == code) {
if (pNode->optrType == TSDB_ALTER_ROLE_PRIVILEGES) {
char privSet[PRIV_GROUP_CNT * 20] = {0};
int32_t len = 0;
for (int32_t i = 0; i < PRIV_GROUP_CNT; ++i) {
len += snprintf(privSet + len, sizeof(privSet) - len, "%" PRIu64 ",", pNode->privileges.privSet.set[i]);
}
if (len > 0) privSet[len - 1] = '\0'; // remove last comma
code = tjsonAddStringToObject(pJson, jkGrantStmtPrivileges, privSet);
} else if (pNode->optrType == TSDB_ALTER_ROLE_ROLE) {
code = tjsonAddStringToObject(pJson, jkRoleStmtRoleName, pNode->roleName);
}
}
return code;
}
static int32_t jsonToGrantStmt(const SJson* pJson, void* pObj) {
SGrantStmt* pNode = (SGrantStmt*)pObj;
int32_t code = tjsonGetStringValue(pJson, jkGrantStmtUserName, pNode->principal);
if (TSDB_CODE_SUCCESS == code) {
code = tjsonGetStringValue(pJson, jkGrantStmtObjName, pNode->objName);
}
if (TSDB_CODE_SUCCESS == code) {
code = tjsonGetStringValue(pJson, jkNameTableName, pNode->tabName);
}
if (TSDB_CODE_SUCCESS == code) {
tjsonGetNumberValue(pJson, "optrType", pNode->optrType, code);
}
if (TSDB_CODE_SUCCESS == code) {
if (pNode->optrType == TSDB_ALTER_ROLE_PRIVILEGES) {
char privSet[256] = {0};
code = tjsonGetStringValue2(pJson, jkGrantStmtPrivileges, privSet, sizeof(privSet));
if (TSDB_CODE_SUCCESS == code) {
int32_t split_num = 0;
char** split = strsplit(privSet, ",", &split_num);
if (!split) {
nodesError("failed at line %d to parse privileges string: %s", __LINE__, privSet);
return TSDB_CODE_INVALID_PARA;
}
for (int32_t i = 0; i < PRIV_GROUP_CNT; ++i) {
if (i < split_num) {
uint64_t value = 0;
if ((code = taosStr2Uint64(split[i], &value))) {
taosMemoryFree(split);
return code;
}
pNode->privileges.privSet.set[i] = value;
} else {
pNode->privileges.privSet.set[i] = 0;
}
}
taosMemoryFree(split);
}
} else if (pNode->optrType == TSDB_ALTER_ROLE_ROLE) {
code = tjsonGetStringValue(pJson, jkRoleStmtRoleName, pNode->roleName);
}
}
return code;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request enhances privilege parsing and checking for CREATE STREAM statements. The changes in parAstParser.c and parAuthenticator.c correctly add privilege checks for CREATE STREAM operations, including handling cases where a target table is not specified. However, a critical security vulnerability has been introduced in nodesCodeFuncs.c by removing a buffer overflow check when serializing privileges to JSON. Additionally, there is some redundant code in parAstParser.c that should be cleaned up.
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.