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

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

Conversation

Zhdanov0
Copy link
Contributor

@Zhdanov0 Zhdanov0 commented Oct 16, 2023

Added the ability to change sql security opt without specifiying the entire body of the function/procedure/package.

  1. ALTER FUNCTION [ {DETERMINISTIC | NOT DETERMINISTIC} ] [ SQL SECURITY {DEFINER | INVOKER} | DROP SQL SECURITY ]
    Examples:
    ALTER FUNCTION name DETERMINISTIC SQL SECURITY DEFINER
    ALTER FUNCTION name SQL SECURITY INVOKER
    ALTER FUNCTION name NOT DETERMINISTIC DROP SQL SECURITY
  2. ALTER PROCEDURE SQL SECURITY {DEFINER | INVOKER} | DROP SQL SECURITY
  3. ALTER PACKAGE SQL SECURITY {DEFINER | INVOKER} | DROP SQL SECURITY

@Zhdanov0
Copy link
Contributor Author

Added the ability to change sql security (definer / invoker / drop) to the existing syntax ALTER FUNCTION { DETERMINISTIC | NOT DETERMINISTIC }.
So that you can write ALTER FUNCTION funcname DETERMINISTIC SQL SECURITY DEFINER

I did it by analogy with "alter_domain_ops"

The deterministic field type was changed from bool to TriState.
The third "null" state appears if you write: ALTER FUNCTION funcname SQL SECURITY DEFINER. Then, with a partial update, deterministic_flag is not set to false.
If you do not change it to TriState, then ALTER FUNCTION funcname SQL SECURITY DEFINER
will set deterministic_flag to false, because in createAlterFunctionNode it defaults to false.

The sql security field type was changed from TriState to std::optional because the SS_DROP state was added.
Added sql security clauses:
states for full update: definer / invoker / nothing
for partial update: definer / invoker / drop
In full update prohibited "drop sql security"
in partial update prohibited "nothing" because it will be infinite recursion.

@sim1984
Copy link

sim1984 commented Oct 17, 2023

SQL SECURITY must be able to change in procedures too. Moreover, it is desirable to have a similar syntax.

@sim1984
Copy link

sim1984 commented Oct 18, 2023

See also #7744

@dyemanov
Copy link
Member

I'm fine with this implementation, please go ahead for stored procedures.

@Zhdanov0 Zhdanov0 changed the title Added the ability to change sql security option in ALTER FUNCTION Added the ability to change sql security option without specifying the entire body of the function/procedure/package Oct 25, 2023
@@ -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)

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 (!executeAlter(tdbb, dsqlScratch, transaction))
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))

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)

src/dsql/parse.y Outdated
@@ -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
	;

src/dsql/parse.y Outdated
@@ -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

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
=============================================================================

Comment on lines 2758 to 2760
{
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;

@dyemanov
Copy link
Member

dyemanov commented Nov 8, 2023

Unless there will be other issues posted, I'll merge this PR later this week.

@dyemanov
Copy link
Member

@Zhdanov0, please resolve the existing conflicts.

@pavel-zotov
Copy link

QA is covered by test for #7744

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants