Skip to content
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

[Feature] Support security view #54458

Merged
merged 3 commits into from
Jan 6, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import com.starrocks.catalog.RangePartitionInfo;
import com.starrocks.catalog.Table;
import com.starrocks.catalog.Type;
import com.starrocks.catalog.View;
import com.starrocks.common.AnalysisException;
import com.starrocks.common.DdlException;
import com.starrocks.common.ErrorCode;
Expand Down Expand Up @@ -205,6 +206,12 @@ public Void visitAlterViewStatement(AlterViewStmt statement, ConnectContext cont

this.db = db;
this.table = table;

if (statement.getAlterClause() == null) {
((View) table).setSecurity(statement.isSecurity());
return null;
}

AlterViewClause alterViewClause = (AlterViewClause) statement.getAlterClause();
visit(alterViewClause, context);
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@

import com.google.common.collect.Maps;
import com.starrocks.analysis.TableName;
import com.starrocks.catalog.BasicTable;
import com.starrocks.catalog.Column;
import com.starrocks.catalog.InternalCatalog;
import com.starrocks.catalog.Table;
import com.starrocks.catalog.View;
import com.starrocks.catalog.system.SystemTable;
import com.starrocks.connector.metadata.MetadataTable;
import com.starrocks.qe.ConnectContext;
import com.starrocks.server.GlobalStateMgr;
import com.starrocks.sql.StatementPlanner;
import com.starrocks.sql.analyzer.Authorizer;
import com.starrocks.sql.ast.AstTraverser;
Expand Down Expand Up @@ -147,6 +149,18 @@ public static void check(ConnectContext context, QueryStatement stmt, List<Table
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(
InternalCatalog.DEFAULT_INTERNAL_CATALOG_NAME, t.getDb(), t.getTbl());

Authorizer.checkAnyActionOnTableLikeObject(context.getCurrentUserIdentity(),
null, t.getDb(), basicTable);
}
}

Authorizer.checkViewAction(context.getCurrentUserIdentity(), context.getCurrentRoleIds(),
tableName, PrivilegeType.SELECT);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
        }
    }
}

Expand Down
11 changes: 11 additions & 0 deletions fe/fe-core/src/main/java/com/starrocks/catalog/View.java
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ public class View extends Table {
@SerializedName(value = "m")
private long sqlMode = 0L;

@SerializedName(value = "s")
private boolean security = false;

// cache used table names
private List<TableName> tableRefsCache = Lists.newArrayList();

Expand Down Expand Up @@ -136,6 +139,14 @@ public long getSqlMode() {
return sqlMode;
}

public void setSecurity(boolean security) {
this.security = security;
}

public boolean isSecurity() {
return security;
}

/**
* Initializes the originalViewDef, inlineViewDef, and queryStmt members
* by parsing the expanded view definition SQL-string.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4242,6 +4242,11 @@ public void createView(CreateViewStmt stmt) throws DdlException {
view.setInlineViewDefWithSqlMode(stmt.getInlineViewDef(),
ConnectContext.get().getSessionVariable().getSqlMode());
// init here in case the stmt string from view.toSql() has some syntax error.

if (stmt.isSecurity()) {
view.setSecurity(stmt.isSecurity());
}

try {
view.init();
} catch (StarRocksException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ public Void visitAlterViewStatement(AlterViewStmt stmt, ConnectContext context)
throw new SemanticException("The specified table [" + tableName + "] is not a view");
}

if (stmt.getAlterClause() == null) {
return null;
}

AlterClause alterClause = stmt.getAlterClause();
AlterViewClause alterViewClause = (AlterViewClause) alterClause;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@
// Alter view statement
public class AlterViewStmt extends DdlStmt {
private final TableName tableName;
private final boolean security;
private final AlterClause alterClause;

public AlterViewStmt(TableName tableName, AlterClause alterClause, NodePosition pos) {
public AlterViewStmt(TableName tableName, boolean security, AlterClause alterClause, NodePosition pos) {
super(pos);
this.tableName = tableName;
this.security = security;
this.alterClause = alterClause;
}

Expand All @@ -34,13 +36,17 @@ public static AlterViewStmt fromReplaceStmt(CreateViewStmt stmt) {
alterViewClause.setInlineViewDef(stmt.getInlineViewDef());
alterViewClause.setColumns(stmt.getColumns());
alterViewClause.setComment(stmt.getComment());
return new AlterViewStmt(stmt.getTableName(), alterViewClause, NodePosition.ZERO);
return new AlterViewStmt(stmt.getTableName(), stmt.isSecurity(), alterViewClause, NodePosition.ZERO);
}

public TableName getTableName() {
return tableName;
}

public boolean isSecurity() {
return security;
}

public AlterClause getAlterClause() {
return alterClause;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,19 @@ public class CreateViewStmt extends DdlStmt {
private final boolean ifNotExists;
private final boolean replace;
private final String comment;
private final boolean security;
protected QueryStatement queryStatement;

//Resolved by Analyzer
protected List<Column> columns;
private String inlineViewDef;

public CreateViewStmt(boolean ifNotExists, boolean replace,
TableName tableName, List<ColWithComment> colWithComments,
public CreateViewStmt(boolean ifNotExists,
boolean replace,
TableName tableName,
List<ColWithComment> colWithComments,
String comment,
boolean security,
QueryStatement queryStmt,
NodePosition pos) {
super(pos);
Expand All @@ -44,6 +48,7 @@ public CreateViewStmt(boolean ifNotExists, boolean replace,
this.tableName = tableName;
this.colWithComments = colWithComments;
this.comment = Strings.nullToEmpty(comment);
this.security = security;
this.queryStatement = queryStmt;
}

Expand Down Expand Up @@ -79,6 +84,10 @@ public String getComment() {
return comment;
}

public boolean isSecurity() {
return security;
}

public QueryStatement getQueryStatement() {
return queryStatement;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1616,12 +1616,23 @@ public ParseNode visitCreateViewStatement(StarRocksParser.CreateViewStatementCon
throw new ParsingException(PARSER_ERROR_MSG.conflictedOptions("if not exists", "or replace"),
createPos(context));
}

boolean isSecurity = false;
if (context.SECURITY() != null) {
if (context.NONE() != null) {
isSecurity = false;
} else if (context.INVOKER() != null) {
isSecurity = true;
}
}

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));
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
    }
}

Expand All @@ -1631,13 +1642,25 @@ public ParseNode visitAlterViewStatement(StarRocksParser.AlterViewStatementConte
TableName targetTableName = qualifiedNameToTableName(qualifiedName);

List<ColWithComment> colWithComments = null;
if (context.columnNameWithComment().size() > 0) {
if (!context.columnNameWithComment().isEmpty()) {
colWithComments = visit(context.columnNameWithComment(), ColWithComment.class);
}
QueryStatement queryStatement = (QueryStatement) visit(context.queryStatement());
AlterClause alterClause = new AlterViewClause(colWithComments, queryStatement, createPos(context));

return new AlterViewStmt(targetTableName, alterClause, createPos(context));
boolean isSecurity = false;
if (context.SECURITY() != null) {
if (context.NONE() != null) {
isSecurity = false;
} else if (context.INVOKER() != null) {
isSecurity = true;
}

return new AlterViewStmt(targetTableName, isSecurity, null, createPos(context));
} else {
QueryStatement queryStatement = (QueryStatement) visit(context.queryStatement());
AlterClause alterClause = new AlterViewClause(colWithComments, queryStatement, createPos(context));

return new AlterViewStmt(targetTableName, isSecurity, alterClause, createPos(context));
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -607,12 +607,15 @@ recoverPartitionStatement

createViewStatement
: CREATE (OR REPLACE)? VIEW (IF NOT EXISTS)? qualifiedName
('(' columnNameWithComment (',' columnNameWithComment)* ')')?
comment? AS queryStatement
('(' columnNameWithComment (',' columnNameWithComment)* ')')?
comment?
(SECURITY (NONE | INVOKER))?
AS queryStatement
;

alterViewStatement
: ALTER VIEW qualifiedName ('(' columnNameWithComment (',' columnNameWithComment)* ')')? AS queryStatement
: ALTER VIEW qualifiedName ('(' columnNameWithComment (',' columnNameWithComment)* ')')? AS queryStatement
| ALTER VIEW qualifiedName SET SECURITY (NONE | INVOKER)
;

dropViewStatement
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ INTERMEDIATE: 'INTERMEDIATE';
INTERSECT: 'INTERSECT';
INTERVAL: 'INTERVAL';
INTO: 'INTO';
INVOKER: 'INVOKER';
GIN: 'GIN';
OVERWRITE: 'OVERWRITE';
IS: 'IS';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1491,7 +1491,7 @@ public void testCreateView(@Mocked RESTCatalog restCatalog, @Mocked BaseView bas
};

CreateViewStmt stmt = new CreateViewStmt(false, false, new TableName("catalog", "db", "table"),
Lists.newArrayList(new ColWithComment("k1", "", NodePosition.ZERO)), "", null, NodePosition.ZERO);
Lists.newArrayList(new ColWithComment("k1", "", NodePosition.ZERO)), "", false, null, NodePosition.ZERO);
stmt.setColumns(Lists.newArrayList(new Column("k1", INT)));
metadata.createView(stmt);

Expand Down
105 changes: 105 additions & 0 deletions test/sql/test_view/R/test_security_view
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
-- name: test_security_view
create table t1(c1 bigint, c2 bigint);
-- result:
-- !result
create table t2(c3 bigint, c4 bigint);
-- result:
-- !result
create view v1 as select * from t1, t2;
-- result:
-- !result
create view v2 security invoker as select * from t1, t2;
-- result:
-- !result
create user if not exists u1;
-- result:
-- !result
grant impersonate on user root to u1;
-- result:
-- !result
grant select on view v1 to user u1;
-- result:
-- !result
grant select on view v2 to user u1;
-- result:
-- !result
create user if not exists u2;
-- result:
-- !result
grant impersonate on user root to u2;
-- result:
-- !result
grant select on table t1 to user u2;
-- result:
-- !result
grant select on table t2 to user u2;
-- result:
-- !result
grant select on view v1 to user u2;
-- result:
-- !result
grant select on view v2 to user u2;
-- result:
-- !result
execute as u1 with no revert;
-- result:
-- !result
select * from v1;
-- result:
-- !result
select * from v2;
-- result:
E: (5203, 'Access denied; you need (at least one of) the SELECT privilege(s) on VIEW v2 for this operation. Please ask the admin to grant permission(s) or try activating existing roles using <set [default] role>. Current role(s): NONE. Inactivated role(s): NONE.')
-- !result
execute as root with no revert;
-- result:
-- !result
execute as u2 with no revert;
-- result:
-- !result
select * from v1;
-- result:
-- !result
select * from v2;
-- result:
-- !result
execute as root with no revert;
-- result:
-- !result
alter view v1 set security invoker;
-- result:
-- !result
alter view v2 set security none;
-- result:
-- !result
execute as u1 with no revert;
-- result:
-- !result
select * from v1;
-- result:
E: (5203, 'Access denied; you need (at least one of) the SELECT privilege(s) on VIEW v1 for this operation. Please ask the admin to grant permission(s) or try activating existing roles using <set [default] role>. Current role(s): NONE. Inactivated role(s): NONE.')
-- !result
select * from v2;
-- result:
-- !result
execute as root with no revert;
-- result:
-- !result
execute as u2 with no revert;
-- result:
-- !result
select * from v1;
-- result:
-- !result
select * from v2;
-- result:
-- !result
execute as root with no revert;
-- result:
-- !result
drop user u1;
-- result:
-- !result
drop user u2;
-- result:
-- !result
Loading
Loading