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.

Sign up for GitHub

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

Added the ability to change sql security option without specifying the entire body of the function/procedure/package #7807

Merged
merged 6 commits into from
Nov 10, 2023
+252 −53
Merged

Added the ability to change sql security option without specifying the entire body of the function/procedure/package #7807

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9b823e6
Add ability to change sql security option in alter function
Oct 16, 2023
5b62093
added the ability to change sql security opt in alter procedure
Oct 25, 2023
695082e
added the ability to change sql security option in alter package
Oct 25, 2023
b774645
refactoring rules for partial alters
Oct 30, 2023
93c1b1f
Merge branch 'master' into master
Zhdanov0 Nov 10, 2023
ecc0fec
triState namespace fix
Nov 10, 2023
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
10 changes: 10 additions & 0 deletions doc/sql.extensions/README.ddl.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,3 +608,13 @@ If not specified in the CREATE TABLE statement, the database-level default behav
(Alexander Zhdanov)

ALTER FUNCTION <name> [ {DETERMINISTIC | NOT DETERMINISTIC} ] [ SQL SECURITY {DEFINER | INVOKER} | DROP SQL SECURITY ]

25) Added the ability to change sql security option without specifying the entire body of the procedure
(Alexander Zhdanov)

ALTER PROCEDURE <name> SQL SECURITY {DEFINER | INVOKER} | DROP SQL SECURITY

26) Added the ability to change sql security option without specifying the entire body of the package
(Alexander Zhdanov)

ALTER PACKAGE <name> SQL SECURITY {DEFINER | INVOKER} | DROP SQL SECURITY
71 changes: 66 additions & 5 deletions src/dsql/DdlNodes.epp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2711,7 +2711,7 @@ DdlNode* CreateAlterProcedureNode::dsqlPass(DsqlCompilerScratch* dsqlScratch)
returns[i]->type->resolve(dsqlScratch);

// check SQL SECURITY is not set if procedure declared in package
if (package.hasData() && ssDefiner.isAssigned())
if (package.hasData() && ssDefiner.has_value())
{
ERRD_post(Arg::Gds(isc_sqlerr) << Arg::Num(-204) <<
Arg::Gds(isc_invalid_clause) << Arg::Str("SQL SECURITY for procedures is prohibit in packages"));
Expand Down Expand Up @@ -2742,11 +2742,22 @@ void CreateAlterProcedureNode::execute(thread_db* tdbb, DsqlCompilerScratch* dsq
AutoSavePoint savePoint(tdbb, transaction);
bool altered = false;

const bool alterIndividualParameters = (alter && !(body || external));

// first pass
if (alter)
if (alterIndividualParameters)
{
if (executeAlterIndividualParameters(tdbb, dsqlScratch, transaction, false, true))
altered = true;
else
status_exception::raise(Arg::Gds(isc_dyn_proc_not_found) << Arg::Str(name));
}
else if (alter)
{
if (executeAlter(tdbb, dsqlScratch, transaction, false, true))
{
altered = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{
altered = true;
}
altered = true;

else
{
if (create) // create or alter
Expand All @@ -2760,7 +2771,11 @@ void CreateAlterProcedureNode::execute(thread_db* tdbb, DsqlCompilerScratch* dsq

compile(tdbb, dsqlScratch);

executeAlter(tdbb, dsqlScratch, transaction, true, false); // second pass
// second pass
if (alterIndividualParameters)
executeAlterIndividualParameters(tdbb, dsqlScratch, transaction, true, false);
else
executeAlter(tdbb, dsqlScratch, transaction, true, false);

if (package.isEmpty())
{
Expand Down Expand Up @@ -2909,10 +2924,10 @@ bool CreateAlterProcedureNode::executeAlter(thread_db* tdbb, DsqlCompilerScratch
else
P.RDB$PRIVATE_FLAG.NULL = TRUE;

if (ssDefiner.isAssigned())
if (ssDefiner.has_value())
{
P.RDB$SQL_SECURITY.NULL = FALSE;
P.RDB$SQL_SECURITY = ssDefiner.asBool() ? FB_TRUE : FB_FALSE;
P.RDB$SQL_SECURITY = ssDefiner.value() == SqlSecurity::SS_DEFINER ? FB_TRUE : FB_FALSE;
}
else
P.RDB$SQL_SECURITY.NULL = TRUE;
Expand Down Expand Up @@ -3024,6 +3039,52 @@ bool CreateAlterProcedureNode::executeAlter(thread_db* tdbb, DsqlCompilerScratch
return modified;
}

bool CreateAlterProcedureNode::executeAlterIndividualParameters(thread_db* tdbb, DsqlCompilerScratch* dsqlScratch, jrd_tra* transaction, bool secondPass, bool runTriggers)
{
Attachment* const attachment = transaction->getAttachment();

bool modifed = false;

AutoCacheRequest requestHandle(tdbb, drq_m_prm_prcs2, DYN_REQUESTS);

FOR (REQUEST_HANDLE requestHandle TRANSACTION_HANDLE transaction)
P IN RDB$PROCEDURES
WITH P.RDB$PROCEDURE_NAME EQ name.c_str() AND
P.RDB$PACKAGE_NAME EQUIV NULLIF(package.c_str(), '')
{
if (P.RDB$SYSTEM_FLAG)
{
status_exception::raise(
Arg::Gds(isc_dyn_cannot_mod_sysproc) <<
MetaName(P.RDB$PROCEDURE_NAME));
}

if (!secondPass && runTriggers && package.isEmpty())
{
executeDdlTrigger(tdbb, dsqlScratch, transaction, DTW_BEFORE,
DDL_TRIGGER_ALTER_PROCEDURE, name, NULL);
}

MODIFY P
if (ssDefiner.has_value())
{
if(ssDefiner.value() != SqlSecurity::SS_DROP)
{
P.RDB$SQL_SECURITY.NULL = FALSE;
P.RDB$SQL_SECURITY = ssDefiner.value() == SqlSecurity::SS_DEFINER ? FB_TRUE : FB_FALSE;
}
else
P.RDB$SQL_SECURITY.NULL = TRUE;
}

modifed = true;
END_MODIFY
}
END_FOR

return modifed;
}

void CreateAlterProcedureNode::storeParameter(thread_db* tdbb, DsqlCompilerScratch* dsqlScratch,
jrd_tra* transaction, USHORT parameterType, unsigned pos, ParameterClause* parameter,
const CollectedParameter* collectedParameter)
Expand Down
5 changes: 4 additions & 1 deletion src/dsql/DdlNodes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,9 @@ class CreateAlterProcedureNode : public DdlNode
void executeCreate(thread_db* tdbb, DsqlCompilerScratch* dsqlScratch, jrd_tra* transaction);
bool executeAlter(thread_db* tdbb, DsqlCompilerScratch* dsqlScratch, jrd_tra* transaction,
bool secondPass, bool runTriggers);
bool executeAlterIndividualParameters(thread_db* tdbb, DsqlCompilerScratch* dsqlScratch, jrd_tra* transaction,
bool secondPass, bool runTriggers);

void storeParameter(thread_db* tdbb, DsqlCompilerScratch* dsqlScratch, jrd_tra* transaction,
USHORT parameterType, unsigned pos, ParameterClause* parameter,
const CollectedParameter* collectedParameter);
Expand All @@ -622,7 +625,7 @@ class CreateAlterProcedureNode : public DdlNode
MetaName packageOwner;
bool privateScope;
bool preserveDefaults;
TriState ssDefiner;
std::optional<SqlSecurity> ssDefiner;
};


Expand Down
60 changes: 55 additions & 5 deletions src/dsql/PackageNodes.epp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,9 @@ string CreateAlterPackageNode::internalPrint(NodePrinter& printer) const

DdlNode* CreateAlterPackageNode::dsqlPass(DsqlCompilerScratch* dsqlScratch)
{
if(alter && !items)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(alter && !items)
if (alter && !items)

return DdlNode::dsqlPass(dsqlScratch);

MemoryPool& pool = dsqlScratch->getPool();

source.ltrim("\n\r\t ");
Expand Down Expand Up @@ -319,9 +322,18 @@ void CreateAlterPackageNode::execute(thread_db* tdbb, DsqlCompilerScratch* dsqlS
// run all statements under savepoint control
AutoSavePoint savePoint(tdbb, transaction);

const bool alterIndividualParameters = (alter && !items);

if (alter)
{
if (!executeAlter(tdbb, dsqlScratch, transaction))
if(alterIndividualParameters)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(alterIndividualParameters)
if (alterIndividualParameters)

{
if(!executeAlterIndividualParameters(tdbb, dsqlScratch, transaction))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(!executeAlterIndividualParameters(tdbb, dsqlScratch, transaction))
if (!executeAlterIndividualParameters(tdbb, dsqlScratch, transaction))

status_exception::raise(
Arg::Gds(isc_no_meta_update) <<
Arg::Gds(isc_dyn_package_not_found) << Arg::Str(name));
}
else if (!executeAlter(tdbb, dsqlScratch, transaction))
{
if (create) // create or alter
executeCreate(tdbb, dsqlScratch, transaction);
Expand Down Expand Up @@ -371,10 +383,10 @@ void CreateAlterPackageNode::executeCreate(thread_db* tdbb, DsqlCompilerScratch*
PKG.RDB$PACKAGE_HEADER_SOURCE.NULL = FALSE;
attachment->storeMetaDataBlob(tdbb, transaction, &PKG.RDB$PACKAGE_HEADER_SOURCE, source);

if (ssDefiner.isAssigned())
if (ssDefiner.has_value())
{
PKG.RDB$SQL_SECURITY.NULL = FALSE;
PKG.RDB$SQL_SECURITY = ssDefiner.asBool() ? FB_TRUE : FB_FALSE;
PKG.RDB$SQL_SECURITY = ssDefiner.value() == SqlSecurity::SS_DEFINER ? FB_TRUE : FB_FALSE;
}
else
PKG.RDB$SQL_SECURITY.NULL = TRUE;
Expand Down Expand Up @@ -445,10 +457,10 @@ bool CreateAlterPackageNode::executeAlter(thread_db* tdbb, DsqlCompilerScratch*
if (!PKG.RDB$VALID_BODY_FLAG.NULL)
PKG.RDB$VALID_BODY_FLAG = FALSE;

if (ssDefiner.isAssigned())
if (ssDefiner.has_value())
{
PKG.RDB$SQL_SECURITY.NULL = FALSE;
PKG.RDB$SQL_SECURITY = ssDefiner.asBool() ? FB_TRUE : FB_FALSE;
PKG.RDB$SQL_SECURITY = ssDefiner.value() == SqlSecurity::SS_DEFINER ? FB_TRUE : FB_FALSE;
}
else
PKG.RDB$SQL_SECURITY.NULL = TRUE;
Expand All @@ -475,6 +487,44 @@ bool CreateAlterPackageNode::executeAlter(thread_db* tdbb, DsqlCompilerScratch*
}


bool CreateAlterPackageNode::executeAlterIndividualParameters(thread_db* tdbb, DsqlCompilerScratch* dsqlScratch, jrd_tra* transaction)
{
AutoCacheRequest requestHandle(tdbb, drq_m_prm_pkg, DYN_REQUESTS);
bool modified = false;

FOR (REQUEST_HANDLE requestHandle TRANSACTION_HANDLE transaction)
PKG IN RDB$PACKAGES
WITH PKG.RDB$PACKAGE_NAME EQ name.c_str()
{
modified = true;

executeDdlTrigger(tdbb, dsqlScratch, transaction, DTW_BEFORE,
DDL_TRIGGER_ALTER_PACKAGE, name, NULL);

MODIFY PKG
if (ssDefiner.has_value())
{
if(ssDefiner.value() != SqlSecurity::SS_DROP)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(ssDefiner.value() != SqlSecurity::SS_DROP)
if (ssDefiner.value() != SqlSecurity::SS_DROP)

{
PKG.RDB$SQL_SECURITY.NULL = FALSE;
PKG.RDB$SQL_SECURITY = ssDefiner.value() == SqlSecurity::SS_DEFINER ? FB_TRUE : FB_FALSE;
}
else
PKG.RDB$SQL_SECURITY.NULL = TRUE;
}
END_MODIFY
}
END_FOR

if (modified)
{
executeDdlTrigger(tdbb, dsqlScratch, transaction,
DTW_AFTER, DDL_TRIGGER_ALTER_PACKAGE, name, NULL);
}

return modified;
}

void CreateAlterPackageNode::executeItems(thread_db* tdbb, DsqlCompilerScratch* dsqlScratch,
jrd_tra* transaction)
{
Expand Down
3 changes: 2 additions & 1 deletion src/dsql/PackageNodes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ class CreateAlterPackageNode : public DdlNode
private:
void executeCreate(thread_db* tdbb, DsqlCompilerScratch* dsqlScratch, jrd_tra* transaction);
bool executeAlter(thread_db* tdbb, DsqlCompilerScratch* dsqlScratch, jrd_tra* transaction);
bool executeAlterIndividualParameters(thread_db* tdbb, DsqlCompilerScratch* dsqlScratch, jrd_tra* transaction);
void executeItems(thread_db* tdbb, DsqlCompilerScratch* dsqlScratch, jrd_tra* transaction);

public:
Expand All @@ -110,7 +111,7 @@ class CreateAlterPackageNode : public DdlNode
Firebird::Array<Item>* items;
Firebird::SortedArray<MetaName> functionNames;
Firebird::SortedArray<MetaName> procedureNames;
TriState ssDefiner;
std::optional<SqlSecurity> ssDefiner;

private:
MetaName owner;
Expand Down
41 changes: 39 additions & 2 deletions src/dsql/parse.y
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2707,9 +2707,13 @@ procedure_clause
| external_procedure_clause
;

%type <createAlterProcedureNode> change_opt_procedure_clause
change_opt_procedure_clause
Copy link
Member

Choose a reason for hiding this comment

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

What is this empty rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not empty, the definition is below.

Copy link
Member

Choose a reason for hiding this comment

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

And why you "declared it" here?

%type <createAlterProcedureNode> change_opt_procedure_clause
change_opt_procedure_clause
	;

;

%type <createAlterProcedureNode> psql_procedure_clause
psql_procedure_clause
: procedure_clause_start sql_security_clause_opt AS local_declarations_opt full_proc_block
: procedure_clause_start optional_sql_security_full_alter_clause AS local_declarations_opt full_proc_block
{
$$ = $1;
$$->ssDefiner = $2;
Expand Down Expand Up @@ -2738,6 +2742,17 @@ procedure_clause_start
{ $$ = $2; }
;

%type <createAlterProcedureNode> change_opt_procedure_clause
change_opt_procedure_clause
: symbol_procedure_name
{ $$ = newNode<CreateAlterProcedureNode>(*$1); }
optional_sql_security_partial_alter_clause
{
$$ = $2;
$$->ssDefiner = $3;
}
;

%type <createAlterProcedureNode> alter_procedure_clause
alter_procedure_clause
: procedure_clause
Expand All @@ -2746,6 +2761,12 @@ alter_procedure_clause
$$->alter = true;
$$->create = false;
}
| change_opt_procedure_clause
{
$$ = $1;
$$->alter = true;
$$->create = false;
}
;

%type <createAlterProcedureNode> replace_procedure_clause
Expand Down Expand Up @@ -2956,7 +2977,7 @@ replace_function_clause

%type <createAlterPackageNode> package_clause
package_clause
: symbol_package_name sql_security_clause_opt AS BEGIN package_items_opt END
: symbol_package_name optional_sql_security_full_alter_clause AS BEGIN package_items_opt END
{
CreateAlterPackageNode* node = newNode<CreateAlterPackageNode>(*$1);
node->ssDefiner = $2;
Expand All @@ -2966,6 +2987,16 @@ package_clause
}
;

%type <createAlterPackageNode> change_opt_package_clause
change_opt_package_clause
Copy link
Member

Choose a reason for hiding this comment

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

Maybe change_opt_package_clause and change_opt_procedure_clause could be put directly in the alter rules?
I think these rules has not so great names to be put standalone and they are not reused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made rules change_opt_package_clause and change_opt_procedure_clause to separate full alter and partial alter. I can change the names to partial_alter_package_clause and partial_alter_procedure_clause. This way it will be clearer and will separate full alter and partial alter.

Copy link
Member

Choose a reason for hiding this comment

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

You call it partial and I call it just another form of alter. Every alter is partial, they change what was specified. So my suggestion to just use another branch of the original rules directly.

Copy link
Contributor Author

@Zhdanov0 Zhdanov0 Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

"Every alter is partial, they change what was specified." - is not correct for functions, procedures and packages.
Functions/procedures/packages change all parameters, even those that were not specified. For example:

ALTER FUNCTION func1 (str varchar(5))
RETURNS INT
AS
DECLARE result INTEGER;
BEGIN
result = POSITION('a', str);
RETURN result;
END;

The deterministic and sql security parameters were not specified, but are still being modified
(reset).
I will call this a full alter (the executeAlter method is used).

Now partial alter (executeAlterIndividualParameters method is used):
Here parameters that are not specified will not be modified. Only the individual specified parameters will be changed.
For example:

ALTER FUNCTION name SQL SECURITY INVOKER

The deterministic parameter is not specified here and it is not modified.

My opinion:
This is different behavior and it should be in different rules.
But it is better to change the name to a more clarifying one: partial_alter_package_clause for example.

Copy link
Member

Choose a reason for hiding this comment

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

So we have a big inconsistency with triggers:

SQL> create table t1 (n1 integer)!
SQL> create trigger t1_ins inactive before insert position 4 on t1 SQL SECURITY DEFINER as begin end!

SQL> show trigger t1_ins!
Triggers on Table T1:
T1_INS, Sequence: 4, Type: BEFORE INSERT, Inactive, SQL SECURITY DEFINER
Trigger text:
=============================================================================
as begin end
=============================================================================

SQL> alter trigger t1_ins as begin end!

SQL> show trigger t1_ins!
Triggers on Table T1:
T1_INS, Sequence: 4, Type: BEFORE INSERT, Inactive, SQL SECURITY DEFINER
Trigger text:
=============================================================================
as begin end
=============================================================================

: symbol_package_name optional_sql_security_partial_alter_clause
{
CreateAlterPackageNode* node = newNode<CreateAlterPackageNode>(*$1);
node->ssDefiner = $2;
$$ = node;
}
;

%type <packageItems> package_items_opt
package_items_opt
: package_items
Expand Down Expand Up @@ -3003,6 +3034,12 @@ alter_package_clause
$$->alter = true;
$$->create = false;
}
| change_opt_package_clause
{
$$ = $1;
$$->alter = true;
$$->create = false;
}
;

%type <createAlterPackageNode> replace_package_clause
Expand Down
2 changes: 2 additions & 0 deletions src/jrd/drq.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ enum drq_type_t
drq_s_prms4,
drq_s_prm_src2,
drq_m_prcs2,
drq_m_prm_prcs2, // modify individual procedure parameters (CreateAlterProcedureNode)
drq_e_prms2,
drq_m_trigger2,
drq_e_prcs2,
Expand All @@ -199,6 +200,7 @@ enum drq_type_t
drq_m_pkg_prc, // drop package body
drq_m_pkg_fun, // drop package body
drq_m_pkg, // alter package
drq_m_prm_pkg, // modify individual package parameters
drq_l_pkg_funcs, // lookup packaged functions
drq_l_pkg_func_args, // lookup packaged function arguments
drq_l_pkg_procs, // lookup packaged procedures
Expand Down