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

Fix bugs for GRANT/REVOKE on SCHEMA #2031

Merged
10 changes: 5 additions & 5 deletions contrib/babelfishpg_tsql/sql/ownership.sql
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ GRANT SELECT on sys.babelfish_sysdatabases TO PUBLIC;
-- BABELFISH_SCHEMA_PERMISSIONS
CREATE TABLE sys.babelfish_schema_permissions (
dbid smallint NOT NULL,
schema_name NAME NOT NULL,
object_name NAME NOT NULL,
permission NAME NOT NULL,
grantee NAME NOT NULL,
object_type NAME,
schema_name NAME NOT NULL COLLATE "C",
object_name NAME NOT NULL COLLATE "C",
permission NAME NOT NULL COLLATE "C",
grantee NAME NOT NULL COLLATE "C",
object_type NAME COLLATE "C",
shalinilohia50 marked this conversation as resolved.
Show resolved Hide resolved
PRIMARY KEY(dbid, schema_name, object_name, permission, grantee)
);
shalinilohia50 marked this conversation as resolved.
Show resolved Hide resolved

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -892,11 +892,11 @@ LANGUAGE plpgsql STABLE;
-- BABELFISH_SCHEMA_PERMISSIONS
CREATE TABLE IF NOT EXISTS sys.babelfish_schema_permissions (
dbid smallint NOT NULL,
schema_name NAME NOT NULL,
object_name NAME NOT NULL,
permission NAME NOT NULL,
grantee NAME NOT NULL,
object_type NAME,
schema_name NAME NOT NULL COLLATE "C",
object_name NAME NOT NULL COLLATE "C",
permission NAME NOT NULL COLLATE "C",
grantee NAME NOT NULL COLLATE "C",
object_type NAME COLLATE "C",
PRIMARY KEY(dbid, schema_name, object_name, permission, grantee)
);

Expand Down
35 changes: 23 additions & 12 deletions contrib/babelfishpg_tsql/src/pl_exec-2.c
Original file line number Diff line number Diff line change
Expand Up @@ -3683,7 +3683,6 @@ exec_stmt_grantschema(PLtsql_execstate *estate, PLtsql_stmt_grantschema *stmt)
char *dbname = get_cur_db_name();
char *login = GetUserNameFromId(GetSessionUserId(), false);
shalinilohia50 marked this conversation as resolved.
Show resolved Hide resolved
bool login_is_db_owner;
Oid datdba;
char *rolname;
char *schema_name;
ListCell *lc;
Expand All @@ -3695,43 +3694,55 @@ exec_stmt_grantschema(PLtsql_execstate *estate, PLtsql_stmt_grantschema *stmt)
* sysadmin or login is not the schema owner, then it doesn't have the permission to GRANT/REVOKE.
*/
login_is_db_owner = 0 == strncmp(login, get_owner_of_db(dbname), NAMEDATALEN);
datdba = get_role_oid("sysadmin", false);
schema_name = get_physical_schema_name(dbname, stmt->schema_name);
schemaOid = LookupExplicitNamespace(schema_name, true);

if(schema_name)
{
schemaOid = LookupExplicitNamespace(schema_name, true);
shalinilohia50 marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_SCHEMA),
errmsg("An object or column name is missing or empty. For SELECT INTO statements, verify each column has a name. For other statements, look for empty alias names. Aliases defined as \"\" or [] are not allowed. Change the alias to a valid name.")));
shalinilohia50 marked this conversation as resolved.
Show resolved Hide resolved
}

if (!OidIsValid(schemaOid))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_SCHEMA),
errmsg("schema \"%s\" does not exist",
schema_name)));

if (!is_member_of_role(GetSessionUserId(), datdba) && !login_is_db_owner && !pg_namespace_ownercheck(schemaOid, GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("Cannot find the schema \"%s\", because it does not exist or you do not have permission.", stmt->schema_name)));

foreach(lc1, stmt->privileges)
{
char *priv_name = (char *) lfirst(lc1);
foreach(lc, stmt->grantees)
{
char *grantee_name = (char *) lfirst(lc);
char *user = GetUserNameFromId(GetUserId(), false);
shalinilohia50 marked this conversation as resolved.
Show resolved Hide resolved
Oid role_oid;
bool grantee_is_db_owner;
rolname = get_physical_user_name(dbname, grantee_name);
if (strcmp(grantee_name, "public") != 0)
shalinilohia50 marked this conversation as resolved.
Show resolved Hide resolved
rolname = get_physical_user_name(dbname, grantee_name);
else
rolname = pstrdup("public");
shalinilohia50 marked this conversation as resolved.
Show resolved Hide resolved
role_oid = get_role_oid(rolname, true);
grantee_is_db_owner = 0 == strncmp(grantee_name, get_owner_of_db(dbname), NAMEDATALEN);


if (role_oid == InvalidOid)
if (strcmp(grantee_name, "public") != 0 && role_oid == InvalidOid)
shalinilohia50 marked this conversation as resolved.
Show resolved Hide resolved
ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("Cannot find the principal '%s', because it does not exist or you do not have permission.", grantee_name)));

if (pg_namespace_ownercheck(schemaOid, role_oid) || is_member_of_role(role_oid, datdba) || grantee_is_db_owner)
if ((strcmp(rolname, user) == 0) || pg_namespace_ownercheck(schemaOid, role_oid) || is_member_of_role(role_oid, get_sysadmin_oid()) || grantee_is_db_owner)
Deepesh125 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

How will this work for multi-db?
(strcmp(rolname, user) == 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you suggest a case which might fail?
I have ran the test framework in both single-db and multi-db mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

How is user = public ever possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is not possible. User will not be public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rolname can be public, but not user.

ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("Cannot grant, deny, or revoke permissions to sa, dbo, entity owner, information_schema, sys, or yourself.")));

if (!is_member_of_role(GetSessionUserId(), get_sysadmin_oid()) && !login_is_db_owner && !pg_namespace_ownercheck(schemaOid, GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("Cannot find the schema \"%s\", because it does not exist or you do not have permission.", stmt->schema_name)));

parsetree_list = gen_grantschema_subcmds(schema_name, rolname, stmt->is_grant, stmt->with_grant_option, priv_name);
/* Run all subcommands */
foreach(parsetree_item, parsetree_list)
Expand Down
102 changes: 74 additions & 28 deletions contrib/babelfishpg_tsql/src/pl_handler.c
Original file line number Diff line number Diff line change
Expand Up @@ -3641,8 +3641,14 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
char *permissions[] = {"select", "insert", "update", "references", "delete"};
for(i = 0; i < 5; i++)
{
if ((rol_spec->rolename != NULL) && !check_bbf_schema_for_entry(logical_schema, obj, permissions[i], rol_spec->rolename))
add_entry_to_bbf_schema(logical_schema, obj, permissions[i], rol_spec->rolename, obj_type);
if ((rol_spec->rolename != NULL) && (strcmp(rol_spec->rolename, "public") != 0))
shalinilohia50 marked this conversation as resolved.
Show resolved Hide resolved
{
/*
* If object permission doesn't exist already, add an entry to the catalog.
*/
if (!check_bbf_schema_for_entry(logical_schema, obj, permissions[i], rol_spec->rolename))
add_entry_to_bbf_schema(logical_schema, obj, permissions[i], rol_spec->rolename, obj_type);
}
}
}
break;
Expand All @@ -3657,10 +3663,19 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
char *permissions[] = {"select", "insert", "update", "references", "delete"};
for(i = 0; i < 5; i++)
{
if ((rol_spec->rolename != NULL) && check_bbf_schema_for_entry(logical_schema, "ALL", permissions[i], rol_spec->rolename) && !has_schema_perms)
has_schema_perms = true;
if ((rol_spec->rolename != NULL) && check_bbf_schema_for_entry(logical_schema, obj, permissions[i], rol_spec->rolename))
del_from_bbf_schema(logical_schema, obj, permissions[i], rol_spec->rolename);
if ((rol_spec->rolename != NULL) && (strcmp(rol_spec->rolename, "public") != 0))
{
/*
* 1. If only schema permission exists, don't revoke any permission.
* 2. If only object permission exists, delete entry from the catalog and revoke permission.
* 3. If both schema and object permission exist, don't revoke any permission but delete object
* entry from the catalog.
*/
if (check_bbf_schema_for_entry(logical_schema, "ALL", permissions[i], rol_spec->rolename) && !has_schema_perms)
shalinilohia50 marked this conversation as resolved.
Show resolved Hide resolved
has_schema_perms = true;
if (check_bbf_schema_for_entry(logical_schema, obj, permissions[i], rol_spec->rolename))
del_from_bbf_schema(logical_schema, obj, permissions[i], rol_spec->rolename);
}
}
if (has_schema_perms)
return;
Expand All @@ -3687,8 +3702,16 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
foreach(lc, grant->grantees)
{
RoleSpec *rol_spec = (RoleSpec *) lfirst(lc);
if ((ap->cols == NULL) && (rol_spec->rolename != NULL) && !check_bbf_schema_for_entry(logical_schema, obj, ap->priv_name, rol_spec->rolename))
add_entry_to_bbf_schema(logical_schema, obj, ap->priv_name, rol_spec->rolename, obj_type);
if ((rol_spec->rolename != NULL) && (strcmp(rol_spec->rolename, "public") != 0))
{
/*
* Don't add an entry, if the permission is granted on column list.
*/
if (ap->cols != NULL)
break;
if (!check_bbf_schema_for_entry(logical_schema, obj, ap->priv_name, rol_spec->rolename))
add_entry_to_bbf_schema(logical_schema, obj, ap->priv_name, rol_spec->rolename, obj_type);
}
}
}
else
Expand All @@ -3700,17 +3723,25 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
* 1. If GRANT on schema does not exist, execute REVOKE statement and remove the catalog entry if exists.
* 2. If GRANT on schema exist, only remove the entry from the catalog if exists.
*/
if ((logical_schema != NULL) && (rol_spec->rolename != NULL) && !check_bbf_schema_for_entry(logical_schema, "ALL", ap->priv_name, rol_spec->rolename))
if ((rol_spec->rolename != NULL) && (strcmp(rol_spec->rolename, "public") != 0))
{
if (prev_ProcessUtility)
prev_ProcessUtility(pstmt, queryString, readOnlyTree, context, params,
queryEnv, dest, qc);
else
standard_ProcessUtility(pstmt, queryString, readOnlyTree, context, params,
if ((logical_schema != NULL) && !check_bbf_schema_for_entry(logical_schema, "ALL", ap->priv_name, rol_spec->rolename))
{
if (prev_ProcessUtility)
prev_ProcessUtility(pstmt, queryString, readOnlyTree, context, params,
queryEnv, dest, qc);
else
standard_ProcessUtility(pstmt, queryString, readOnlyTree, context, params,
queryEnv, dest, qc);
}
/*
* Don't remove an entry, if the permission is revoked on column list.
*/
if (ap->cols != NULL)
break;
if (check_bbf_schema_for_entry(logical_schema, obj, ap->priv_name, rol_spec->rolename))
del_from_bbf_schema(logical_schema, obj, ap->priv_name, rol_spec->rolename);
}
if ((ap->cols == NULL) && (rol_spec->rolename != NULL) && check_bbf_schema_for_entry(logical_schema, obj, ap->priv_name, rol_spec->rolename))
del_from_bbf_schema(logical_schema, obj, ap->priv_name, rol_spec->rolename);
}
}
}
Expand Down Expand Up @@ -3761,8 +3792,11 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
foreach(lc, grant->grantees)
{
RoleSpec *rol_spec = (RoleSpec *) lfirst(lc);
if ((rol_spec->rolename != NULL) && !check_bbf_schema_for_entry(logicalschema, funcname, "execute", rol_spec->rolename))
add_entry_to_bbf_schema(logicalschema, funcname, "execute", rol_spec->rolename, obj_type);
if ((rol_spec->rolename != NULL) && (strcmp(rol_spec->rolename, "public") != 0))
{
if (!check_bbf_schema_for_entry(logicalschema, funcname, "execute", rol_spec->rolename))
add_entry_to_bbf_schema(logicalschema, funcname, "execute", rol_spec->rolename, obj_type);
shalinilohia50 marked this conversation as resolved.
Show resolved Hide resolved
}
}
break;
}
Expand All @@ -3772,10 +3806,13 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
{
RoleSpec *rol_spec = (RoleSpec *) lfirst(lc);
bool has_schema_perms = false;
if ((rol_spec->rolename != NULL) && check_bbf_schema_for_entry(logicalschema, "ALL", "execute", rol_spec->rolename) && !has_schema_perms)
has_schema_perms = true;
if ((rol_spec->rolename != NULL) && check_bbf_schema_for_entry(logicalschema, funcname, "execute", rol_spec->rolename))
del_from_bbf_schema(logicalschema, funcname, "execute", rol_spec->rolename);
if ((rol_spec->rolename != NULL) && (strcmp(rol_spec->rolename, "public") != 0))
{
if (check_bbf_schema_for_entry(logicalschema, "ALL", "execute", rol_spec->rolename) && !has_schema_perms)
has_schema_perms = true;
if (check_bbf_schema_for_entry(logicalschema, funcname, "execute", rol_spec->rolename))
del_from_bbf_schema(logicalschema, funcname, "execute", rol_spec->rolename);
}
if (has_schema_perms)
return;
}
Expand All @@ -3798,9 +3835,14 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
foreach(lc, grant->grantees)
{
RoleSpec *rol_spec = (RoleSpec *) lfirst(lc);
/* Don't store a row in catalog, if permission is granted for column */
if ((rol_spec->rolename != NULL) && !check_bbf_schema_for_entry(logicalschema, funcname, ap->priv_name, rol_spec->rolename))
add_entry_to_bbf_schema(logicalschema, funcname, ap->priv_name, rol_spec->rolename, obj_type);
/* Add an entry to the catalog, if an entry doesn't exist already. */
if ((rol_spec->rolename != NULL) && (strcmp(rol_spec->rolename, "public") != 0))
{
if (!check_bbf_schema_for_entry(logicalschema, funcname, ap->priv_name, rol_spec->rolename))
{
add_entry_to_bbf_schema(logicalschema, funcname, ap->priv_name, rol_spec->rolename, obj_type);
}
}
}
}
else
Expand All @@ -3812,18 +3854,22 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
* 1. If GRANT on schema does not exist, execute REVOKE statement and remove the catalog entry if exists.
* 2. If GRANT on schema exist, only remove the entry from the catalog if exists.
*/
if ((rol_spec->rolename != NULL) && !check_bbf_schema_for_entry(logicalschema, "ALL", ap->priv_name, rol_spec->rolename))
if ((rol_spec->rolename != NULL) && (strcmp(rol_spec->rolename, "public") != 0))
{
if (!check_bbf_schema_for_entry(logicalschema, "ALL", ap->priv_name, rol_spec->rolename))
{
/* Execute REVOKE statement. */
if (prev_ProcessUtility)
prev_ProcessUtility(pstmt, queryString, readOnlyTree, context, params,
queryEnv, dest, qc);
else
standard_ProcessUtility(pstmt, queryString, readOnlyTree, context, params,
queryEnv, dest, qc);
}
/* Remove an entry from the catalog, if it exists. */
if (check_bbf_schema_for_entry(logicalschema, funcname, ap->priv_name, rol_spec->rolename))
del_from_bbf_schema(logicalschema, funcname, ap->priv_name, rol_spec->rolename);
}
if ((rol_spec->rolename != NULL) && check_bbf_schema_for_entry(logicalschema, funcname, ap->priv_name, rol_spec->rolename))
del_from_bbf_schema(logicalschema, funcname, ap->priv_name, rol_spec->rolename);
}
}
}
Expand Down
5 changes: 4 additions & 1 deletion contrib/babelfishpg_tsql/src/pltsql_utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -1036,7 +1036,10 @@ update_GrantStmt(Node *n, const char *object, const char *obj_schema, const char
if (grantee && stmt->grantees)
{
RoleSpec *tmp = (RoleSpec *) llast(stmt->grantees);

if (strcmp(grantee, "public") == 0)
{
tmp->roletype = ROLESPEC_PUBLIC;
}
tmp->rolename = pstrdup(grantee);
}

Expand Down
20 changes: 20 additions & 0 deletions contrib/babelfishpg_tsql/src/tsqlIface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5527,6 +5527,11 @@ makeGrantdbStatement(TSqlParser::Security_statementContext *ctx)
char *grantee_name = pstrdup(downcase_truncate_identifier(id_str.c_str(), id_str.length(), true));
grantee_list = lappend(grantee_list, grantee_name);
}
if (prin->PUBLIC())
{
char *grantee_name = (char *)"public";
grantee_list = lappend(grantee_list, grantee_name);
}
}
result->grantees = grantee_list;
return (PLtsql_stmt *) result;
Expand Down Expand Up @@ -5555,6 +5560,11 @@ makeGrantdbStatement(TSqlParser::Security_statementContext *ctx)
char *grantee_name = pstrdup(downcase_truncate_identifier(id_str.c_str(), id_str.length(), true));
grantee_list = lappend(grantee_list, grantee_name);
}
if (prin->PUBLIC())
{
char *grantee_name = (char *)"public";
grantee_list = lappend(grantee_list, grantee_name);
}
}
result->grantees = grantee_list;
return (PLtsql_stmt *) result;
Expand Down Expand Up @@ -5585,6 +5595,11 @@ makeGrantdbStatement(TSqlParser::Security_statementContext *ctx)
char *grantee_name = pstrdup(downcase_truncate_identifier(id_str.c_str(), id_str.length(), true));
grantee_list = lappend(grantee_list, grantee_name);
}
if (prin->PUBLIC())
{
char *grantee_name = (char *)"public";
grantee_list = lappend(grantee_list, grantee_name);
}
}
List *privilege_list = NIL;
for (auto perm: ctx->grant_statement()->permissions()->permission())
Expand Down Expand Up @@ -5637,6 +5652,11 @@ makeGrantdbStatement(TSqlParser::Security_statementContext *ctx)
char *grantee_name = pstrdup(downcase_truncate_identifier(id_str.c_str(), id_str.length(), true));
grantee_list = lappend(grantee_list, grantee_name);
}
if (prin->PUBLIC())
{
char *grantee_name = (char *)"public";
shalinilohia50 marked this conversation as resolved.
Show resolved Hide resolved
grantee_list = lappend(grantee_list, grantee_name);
}
}
List *privilege_list = NIL;
for (auto perm: ctx->revoke_statement()->permissions()->permission())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1704,7 +1704,8 @@ void TsqlUnsupportedFeatureHandlerImpl::checkSupportedGrantStmt(TSqlParser::Gran
continue;
else
{
unsupported_feature = "GRANT PERMISSION " + perm->getText();
unsupported_feature = "GRANT PERMISSION " + ::getFullText(single_perm);
std::transform(unsupported_feature.begin(), unsupported_feature.end(), unsupported_feature.begin(), ::toupper);
handle(INSTR_UNSUPPORTED_TSQL_REVOKE_STMT, unsupported_feature.c_str(), getLineAndPos(perm));
}
}
Expand All @@ -1719,6 +1720,7 @@ void TsqlUnsupportedFeatureHandlerImpl::checkSupportedGrantStmt(TSqlParser::Gran
if (obj_type && !(obj_type->OBJECT() || obj_type->SCHEMA()))
{
unsupported_feature = "GRANT ON " + obj_type->getText();
std::transform(unsupported_feature.begin(), unsupported_feature.end(), unsupported_feature.begin(), ::toupper);
handle(INSTR_UNSUPPORTED_TSQL_REVOKE_STMT, unsupported_feature.c_str(), getLineAndPos(obj_type));
}
}
Expand Down Expand Up @@ -1798,7 +1800,8 @@ void TsqlUnsupportedFeatureHandlerImpl::checkSupportedRevokeStmt(TSqlParser::Rev
continue;
else
{
unsupported_feature = "REVOKE PERMISSION " + perm->getText();
unsupported_feature = "REVOKE PERMISSION " + ::getFullText(single_perm);
std::transform(unsupported_feature.begin(), unsupported_feature.end(), unsupported_feature.begin(), ::toupper);
handle(INSTR_UNSUPPORTED_TSQL_REVOKE_STMT, unsupported_feature.c_str(), getLineAndPos(perm));
}
}
Expand All @@ -1813,6 +1816,7 @@ void TsqlUnsupportedFeatureHandlerImpl::checkSupportedRevokeStmt(TSqlParser::Rev
if (obj_type && !(obj_type->OBJECT() || obj_type->SCHEMA()))
{
unsupported_feature = "REVOKE ON " + obj_type->getText();
std::transform(unsupported_feature.begin(), unsupported_feature.end(), unsupported_feature.begin(), ::toupper);
handle(INSTR_UNSUPPORTED_TSQL_REVOKE_STMT, unsupported_feature.c_str(), getLineAndPos(obj_type));
}
}
Expand Down
Loading