[Feature] Support security view #54458
Conversation
![starrocks-cr[bot]](https://avatars.githubusercontent.com/in/404334?s=60&v=4){kind=small}
| public boolean isSecurity() { | ||
| return security; | ||
| } | ||
|
|
||
| public QueryStatement getQueryStatement() { | ||
| return queryStatement; | ||
| } |
There was a problem hiding this comment.
The most risky bug in this code is: security field might not be initialized, leading to potential security issues or incorrect behavior when its value is accessed.
You can modify the code like this:
private final boolean security;to include a default value if security is optional and should default to false:
private final boolean security = false;Furthermore, ensure that security is always initialized in the constructor. Since it appears security should already be passed as an argument, verify that all calls to the constructor now correctly include the security argument. If any call doesn’t or incorrectly handles security logic elsewhere, make sure to update those instances accordingly.
| return new CreateViewStmt( | ||
| context.IF() != null, | ||
| context.REPLACE() != null, | ||
| targetTableName, | ||
| colWithComments, | ||
| context.comment() == null ? null : ((StringLiteral) visit(context.comment())).getStringValue(), | ||
| isSecurity, | ||
| (QueryStatement) visit(context.queryStatement()), createPos(context)); | ||
| } | ||
|
|
There was a problem hiding this comment.
The most risky bug in this code is:
The isSecurity flag defaults to false, which may not correctly capture cases where both SECURITY and INVOKER contexts are absent.
You can modify the code like this:
boolean isSecurity = false;
if (context.SECURITY() != null) {
if (context.NONE() != null) {
isSecurity = false;
} else if (context.INVOKER() != null) {
isSecurity = true;
} else {
throw new ParsingException(PARSER_ERROR_MSG.invalidOption("SECURITY"), createPos(context));
}
}
| null, t.getDb(), basicTable); | ||
| } | ||
| } | ||
|
|
||
| Authorizer.checkViewAction(context.getCurrentUserIdentity(), context.getCurrentRoleIds(), | ||
| tableName, PrivilegeType.SELECT); | ||
| } |
There was a problem hiding this comment.
The most risky bug in this code is:
Improper handling of null return value from getBasicTable, which may lead to a NullPointerException.
You can modify the code like this:
public static void check(ConnectContext context, QueryStatement stmt, List<TableRef> tableRefs) {
for (TableRef tableRef : tableRefs) {
TableName tableName = tableRef.getName();
Table table = viewAnalyzer.getOrAnalyze(context, tableName);
if (table instanceof MetadataTable || table instanceof SystemTable) {
Authorizer.checkTableAction(context.getCurrentUserIdentity(), context.getCurrentRoleIds(),
tableName, PrivilegeType.SELECT);
} else {
View view = (View) table;
if (view.isSecurity()) {
List<TableName> allTables = view.getTableRefs();
for (TableName t : allTables) {
BasicTable basicTable = GlobalStateMgr.getCurrentState().getMetadataMgr().getBasicTable(
t.getCatalog(), t.getDb(), t.getTbl());
if (basicTable != null) { // Add null check
Authorizer.checkAnyActionOnTableLikeObject(context.getCurrentUserIdentity(),
null, t.getDb(), basicTable);
} else {
// Handle the case where basicTable is null, e.g., log an error or throw an exception.
}
}
}
Authorizer.checkViewAction(context.getCurrentUserIdentity(), context.getCurrentRoleIds(),
tableName, PrivilegeType.SELECT);
}
}
}
HangyuanLiu
force-pushed
the
1216-security-view
branch
from
49b0ed8 to
3bb5188
Compare
HangyuanLiu
force-pushed
the
1216-security-view
branch
from
3bb5188 to
c405d7f
Compare
|
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]❌ fail : 11 / 25 (44.00%) file detail
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
Why I'm doing:
What I'm doing:
Fixes #54459
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: