incorrect error message, while dropping PROCEDURE
Hi,
Currently if some one try to drop the PROCEDURE and
it don't have privilege or it's not an owner, than error message
still indicate object as FUNCTION.
Example:
postgres@37737=#drop procedure pro;
ERROR: must be owner of function pro
This doesn't look correct specially that now we have separate
object type as OBJECT_PROCEDURE. It seems like we need
to introduce new AclObjectKind for PROCEDURE and do the
necessary changes to pass the correct AclObjectKind.
PFA patch, where introduced new AclObjectKind (ACL_KIND_PROCEDURE),
msg for the new AclObjectKind, and passed it through at
appropriate places.
Also update the necessary "make check" expected output changes.
Regards,
Thanks,
Rushabh Lathia
www.EnterpriseDB.com
Attachments:
AclObjectKind_procedure.patchtext/x-patch; charset=US-ASCII; name=AclObjectKind_procedure.patchDownload
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index e481cf3..680ef18 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -257,6 +257,7 @@ restrict_and_check_grant(bool is_grant, AclMode avail_goptions, bool all_privs,
whole_mask = ACL_ALL_RIGHTS_DATABASE;
break;
case ACL_KIND_PROC:
+ case ACL_KIND_PROCEDURE:
whole_mask = ACL_ALL_RIGHTS_FUNCTION;
break;
case ACL_KIND_LANGUAGE:
@@ -2565,7 +2566,8 @@ ExecGrant_Function(InternalGrant *istmt)
this_privileges =
restrict_and_check_grant(istmt->is_grant, avail_goptions,
istmt->all_privs, istmt->privileges,
- funcId, grantorId, ACL_KIND_PROC,
+ funcId, grantorId,
+ (istmt->objtype == ACL_OBJECT_PROCEDURE) ? ACL_KIND_PROCEDURE : ACL_KIND_PROC,
NameStr(pg_proc_tuple->proname),
0, NULL);
@@ -3360,6 +3362,8 @@ static const char *const no_priv_msg[MAX_ACL_KIND] =
gettext_noop("permission denied for database %s"),
/* ACL_KIND_PROC */
gettext_noop("permission denied for function %s"),
+ /* ACL_KIND_PROCEDURE */
+ gettext_noop("permission denied for procedure %s"),
/* ACL_KIND_OPER */
gettext_noop("permission denied for operator %s"),
/* ACL_KIND_TYPE */
@@ -3412,6 +3416,8 @@ static const char *const not_owner_msg[MAX_ACL_KIND] =
gettext_noop("must be owner of database %s"),
/* ACL_KIND_PROC */
gettext_noop("must be owner of function %s"),
+ /* ACL_KIND_PROCEDURE */
+ gettext_noop("must be owner of procedure %s"),
/* ACL_KIND_OPER */
gettext_noop("must be owner of operator %s"),
/* ACL_KIND_TYPE */
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 9553675..d67a68b 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2261,7 +2261,7 @@ check_object_ownership(Oid roleid, ObjectType objtype, ObjectAddress address,
case OBJECT_PROCEDURE:
case OBJECT_ROUTINE:
if (!pg_proc_ownercheck(address.objectId, roleid))
- aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PROC,
+ aclcheck_error(ACLCHECK_NOT_OWNER, objtype == OBJECT_PROCEDURE ? ACL_KIND_PROCEDURE : ACL_KIND_PROC,
NameListToString((castNode(ObjectWithArgs, object))->objname));
break;
case OBJECT_OPERATOR:
diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index 2a9c901..7541146 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -2270,7 +2270,7 @@ ExecuteCallStmt(ParseState *pstate, CallStmt *stmt)
aclresult = pg_proc_aclcheck(fexpr->funcid, GetUserId(), ACL_EXECUTE);
if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, ACL_KIND_PROC, get_func_name(fexpr->funcid));
+ aclcheck_error(aclresult, ACL_KIND_PROCEDURE, get_func_name(fexpr->funcid));
InvokeFunctionExecuteHook(fexpr->funcid);
nargs = list_length(fexpr->args);
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 254a811..44037a0 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -191,6 +191,7 @@ typedef enum AclObjectKind
ACL_KIND_SEQUENCE, /* pg_sequence */
ACL_KIND_DATABASE, /* pg_database */
ACL_KIND_PROC, /* pg_proc */
+ ACL_KIND_PROCEDURE, /* pg_proc procedure */
ACL_KIND_OPER, /* pg_operator */
ACL_KIND_TYPE, /* pg_type */
ACL_KIND_LANGUAGE, /* pg_language */
diff --git a/src/test/regress/expected/create_procedure.out b/src/test/regress/expected/create_procedure.out
index 5538ef2..9a5be38 100644
--- a/src/test/regress/expected/create_procedure.out
+++ b/src/test/regress/expected/create_procedure.out
@@ -73,7 +73,7 @@ GRANT INSERT ON cp_test TO regress_user1;
REVOKE EXECUTE ON PROCEDURE ptest1(text) FROM PUBLIC;
SET ROLE regress_user1;
CALL ptest1('a'); -- error
-ERROR: permission denied for function ptest1
+ERROR: permission denied for procedure ptest1
RESET ROLE;
GRANT EXECUTE ON PROCEDURE ptest1(text) TO regress_user1;
SET ROLE regress_user1;
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index e6994f0..c92993d 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -696,7 +696,7 @@ ERROR: permission denied for function testfunc1
SELECT testagg1(x) FROM (VALUES (1), (2), (3)) _(x); -- fail
ERROR: permission denied for function testagg1
CALL testproc1(6); -- fail
-ERROR: permission denied for function testproc1
+ERROR: permission denied for procedure testproc1
SELECT col1 FROM atest2 WHERE col2 = true; -- fail
ERROR: permission denied for relation atest2
SELECT testfunc4(true); -- ok
@@ -724,7 +724,7 @@ ERROR: must be owner of function testfunc1
DROP AGGREGATE testagg1(int); -- fail
ERROR: must be owner of function testagg1
DROP PROCEDURE testproc1(int); -- fail
-ERROR: must be owner of function testproc1
+ERROR: must be owner of procedure testproc1
\c -
DROP FUNCTION testfunc1(int); -- ok
-- restore to sanity
On 12/13/17 23:31, Rushabh Lathia wrote:
Currently if some one try to drop the PROCEDURE and
it don't have privilege or it's not an owner, than error message
still indicate object as FUNCTION.
Yes, that is actually something that is fixed by the patches proposed in
the thread "replace GrantObjectType with ObjectType".
PFA patch, where introduced new AclObjectKind (ACL_KIND_PROCEDURE),
msg for the new AclObjectKind, and passed it through at
appropriate places.
Yeah, that's a way to do it, but having both ACL_KIND_PROC and
ACL_KIND_PROCEDURE is clearly confusing. The above-mentioned patch
cleans that up more thoroughly, I think.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Dec 15, 2017 at 12:18 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 12/13/17 23:31, Rushabh Lathia wrote:
PFA patch, where introduced new AclObjectKind (ACL_KIND_PROCEDURE),
msg for the new AclObjectKind, and passed it through at
appropriate places.Yeah, that's a way to do it, but having both ACL_KIND_PROC and
ACL_KIND_PROCEDURE is clearly confusing. The above-mentioned patch
cleans that up more thoroughly, I think.
+1. I looked at the patch of Peter, as well as this one, and I prefer
Peter's approach of reducing duplications in concepts.
--
Michael
On Fri, Dec 15, 2017 at 3:32 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:
On Fri, Dec 15, 2017 at 12:18 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:On 12/13/17 23:31, Rushabh Lathia wrote:
PFA patch, where introduced new AclObjectKind (ACL_KIND_PROCEDURE),
msg for the new AclObjectKind, and passed it through at
appropriate places.Yeah, that's a way to do it, but having both ACL_KIND_PROC and
ACL_KIND_PROCEDURE is clearly confusing. The above-mentioned patch
cleans that up more thoroughly, I think.+1. I looked at the patch of Peter, as well as this one, and I prefer
Peter's approach of reducing duplications in concepts.
Yes, I also agree.
Thanks,
--
Rushabh Lathia