incorrect error message, while dropping PROCEDURE

Started by Rushabh Lathiaabout 8 years ago4 messages
#1Rushabh Lathia
rushabh.lathia@gmail.com
1 attachment(s)

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
#2Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Rushabh Lathia (#1)
Re: incorrect error message, while dropping PROCEDURE

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

#3Michael Paquier
michael.paquier@gmail.com
In reply to: Peter Eisentraut (#2)
Re: incorrect error message, while dropping PROCEDURE

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
#4Rushabh Lathia
rushabh.lathia@gmail.com
In reply to: Michael Paquier (#3)
Re: incorrect error message, while dropping PROCEDURE

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