Fixed CORE-5248: Improve consistency in GRANT syntax between roles an…

…d privileges according to SQL standard

src/dsql/DdlNodes.epp

@@ -85,6 +85,8 @@ static bool fieldExists(thread_db* tdbb, jrd_tra* transaction, const MetaName& r
 	const MetaName& fieldName);
 static bool isItSqlRole(thread_db* tdbb, jrd_tra* transaction, const MetaName& inputName,
 	MetaName& outputName);
+static int getGrantorOption(thread_db* tdbb, jrd_tra* transaction, const MetaName& grantor,
+	int grantorType, const MetaName& roleName);
 static MetaName getIndexRelationName(thread_db* tdbb, jrd_tra* transaction,
 	const MetaName& indexName);
 static const char* getRelationScopeName(const rel_t type);
@@ -10577,6 +10579,9 @@ void DropRoleNode::execute(thread_db* tdbb, DsqlCompilerScratch* dsqlScratch, jr
 		ERASE ROL;
 
 		found = true;
+
+		if (!ROL.RDB$SECURITY_CLASS.NULL)
+			deleteSecurityClass(tdbb, transaction, ROL.RDB$SECURITY_CLASS);
 	}
 	END_FOR
 
@@ -11129,9 +11134,6 @@ void GrantRevokeNode::grantRevoke(thread_db* tdbb, jrd_tra* transaction, const G
 		}
 	}
 
-	MetaName grantorRevoker(grantor ?
-		*grantor : tdbb->getAttachment()->att_user->getUserName());
-
 	if (grantor && !tdbb->getAttachment()->locksmith(tdbb, USE_GRANTED_BY_CLAUSE))
 	{
 		const Firebird::MetaName& owner(tdbb->getDatabase()->dbb_owner);
@@ -11142,6 +11144,9 @@ void GrantRevokeNode::grantRevoke(thread_db* tdbb, jrd_tra* transaction, const G
 			(Arg::PrivateDyn(295) << DBA_USER_NAME << owner).raise();
 	}
 
+	MetaName currentUser(tdbb->getAttachment()->att_user->getUserName());
+	MetaName grantorRevoker(grantor ? *grantor : currentUser);
+
 	if (!isGrant && !privs)	// REVOKE ALL ON ALL
 	{
 		AutoCacheRequest request(tdbb, drq_e_grant3, DYN_REQUESTS);
@@ -11345,7 +11350,14 @@ void GrantRevokeNode::grantRevoke(thread_db* tdbb, jrd_tra* transaction, const G
 					// columns. So for all fields in this table which have been granted the
 					// privilege, we erase the entries from RDB$USER_PRIVILEGES.
 
-					if (grantorRevoker == PRIV.RDB$GRANTOR)
+					MetaName owner;
+					if ((grantorRevoker == PRIV.RDB$GRANTOR) ||
+						((objType == obj_sql_role) && (PRIV.RDB$PRIVILEGE[0] == 'M') &&		// This is ROLE to USER grant
+						 (currentUser != user) &&						// And current user does not revoke his own grant
+						 ((isItSqlRole(tdbb, transaction, objName, owner) &&		// Pick up role owner name
+						   (tdbb->getAttachment()->locksmith(tdbb, GRANT_REVOKE_ON_ANY_OBJECT) ||	// God-like check
+						    (owner == currentUser))) ||					// Current user is role owner
+						  (getGrantorOption(tdbb, transaction, currentUser, obj_user, objName) == 2))))	// or has ADMIN option
 					{
 						curOptions = PRIV.RDB$GRANT_OPTION;
 						curField = PRIV.RDB$FIELD_NAME;

src/jrd/grant.epp

@@ -504,6 +504,21 @@ static void get_object_info(thread_db* tdbb,
 		}
 		END_FOR
 	}
+	else if (obj_type == obj_sql_role)
+	{
+		AutoCacheRequest request(tdbb, irq_grant19, IRQ_REQUESTS);
+
+		FOR(REQUEST_HANDLE request)
+			ROL IN RDB$ROLES WITH
+				ROL.RDB$ROLE_NAME EQ object_name
+		{
+			s_class = ROL.RDB$SECURITY_CLASS;
+			default_class = "";
+			owner = ROL.RDB$OWNER_NAME;
+			view = false;
+		}
+		END_FOR
+	}
 	else
 	{
 		s_class = get_object_name(obj_type);
@@ -546,7 +561,10 @@ static void get_user_privs(thread_db*					tdbb,
 			PRV.RDB$OBJECT_TYPE EQ obj_type AND
 			(PRV.RDB$USER NE "PUBLIC" OR PRV.RDB$USER_TYPE NE obj_user) AND
 			(PRV.RDB$USER NE owner.c_str() OR PRV.RDB$USER_TYPE NE obj_user) AND
-			PRV.RDB$FIELD_NAME MISSING
+			((PRV.RDB$OBJECT_TYPE NE obj_sql_role AND
+			  PRV.RDB$FIELD_NAME MISSING) OR
+			 (PRV.RDB$OBJECT_TYPE EQ obj_sql_role AND
+			  PRV.RDB$GRANT_OPTION EQ WITH_ADMIN_OPTION))
 			SORTED BY PRV.RDB$USER, PRV.RDB$USER_TYPE
 	{
 		fb_utils::exact_name_limit(PRV.RDB$USER, sizeof(PRV.RDB$USER));
@@ -567,7 +585,7 @@ static void get_user_privs(thread_db*					tdbb,
 			}
 			user = PRV.RDB$USER;
 		}
-		priv |= trans_sql_priv(PRV.RDB$PRIVILEGE);
+		priv |= trans_sql_priv(obj_type == obj_sql_role ? "O" : PRV.RDB$PRIVILEGE);
 	}
 	END_FOR
 
@@ -591,6 +609,8 @@ static void grant_user(Acl& acl,
  *	Grant privileges to a particular user.
  *
  **************************************/
+	Acl::size_type back = acl.getCount();
+
 	CHECK_AND_MOVE(acl, ACL_id_list);
 	switch (user_type)
 	{
@@ -642,7 +662,8 @@ static void grant_user(Acl& acl,
 		acl.add(reinterpret_cast<const UCHAR*>(user.c_str()), length);
 	}
 
-	SCL_move_priv(privs, acl);
+	if (!SCL_move_priv(privs, acl))
+		acl.shrink(back);
 }
 
 

src/jrd/irq.h

@@ -175,6 +175,7 @@ enum irq_type_t
 	irq_grant16,			// process grant option (domains)
 	irq_grant17,			// process grant option (database)
 	irq_grant18,			// process grant option (filters)
+	irq_grant19,			// process grant option (roles)
 	irq_l_curr_format,		// lookup table's current format
 
 	irq_linger,				// get database linger value

src/jrd/scl.epp

@@ -1157,7 +1157,7 @@ void UserId::sclInit(thread_db* tdbb, bool create, const UserId& tempId)
 }
 
 
-void SCL_move_priv(SecurityClass::flags_t mask, Acl& acl)
+bool SCL_move_priv(SecurityClass::flags_t mask, Acl& acl)
 {
 /**************************************
  *
@@ -1174,16 +1174,19 @@ void SCL_move_priv(SecurityClass::flags_t mask, Acl& acl)
 	acl.push(ACL_end);
 	acl.push(ACL_priv_list);
 
+	bool rc = false;
 	for (const P_NAMES* priv = p_names; priv->p_names_priv; priv++)
 	{
 		if (mask & priv->p_names_priv)
 		{
 			fb_assert(priv->p_names_acl <= MAX_UCHAR);
 			acl.push(priv->p_names_acl);
+			rc = true;
 		}
 	}
 
 	acl.push(0);
+	return rc;
 }
 
 

src/jrd/scl_proto.h

@@ -63,7 +63,7 @@ USHORT SCL_convert_privilege(Jrd::thread_db* tdbb, Jrd::jrd_tra* transaction, co
 namespace Jrd {
 typedef Firebird::Array<UCHAR> Acl;
 }
-void SCL_move_priv(Jrd::SecurityClass::flags_t, Jrd::Acl&);
+bool SCL_move_priv(Jrd::SecurityClass::flags_t, Jrd::Acl&);
 
 
 #endif // JRD_SCL_PROTO_H

src/jrd/vio.cpp

@@ -1874,7 +1874,7 @@ void VIO_erase(thread_db* tdbb, record_param* rpb, jrd_tra* transaction)
 		EVL_field(0, rpb->rpb_record, f_prv_rname, &desc);
 		MOV_get_metaname(&desc, object_name);
 		EVL_field(0, rpb->rpb_record, f_prv_grant, &desc2);
-		if (MOV_get_long(&desc2, 0))
+		if (MOV_get_long(&desc2, 0) == WITH_GRANT_OPTION)		// ADMIN option should not cause cascade
 		{
 			EVL_field(0, rpb->rpb_record, f_prv_user, &desc2);
 			MetaName revokee;
@@ -3112,7 +3112,6 @@ void VIO_store(thread_db* tdbb, record_param* rpb, jrd_tra* transaction)
 		case rel_rcon:
 		case rel_refc:
 		case rel_ccon:
-		case rel_roles:
 		case rel_sec_users:
 		case rel_sec_user_attributes:
 		case rel_msgs:
@@ -3125,6 +3124,12 @@ void VIO_store(thread_db* tdbb, record_param* rpb, jrd_tra* transaction)
 			protect_system_table_insert(tdbb, request, relation);
 			break;
 
+		case rel_roles:
+			protect_system_table_insert(tdbb, request, relation);
+			EVL_field(0, rpb->rpb_record, f_rol_name, &desc);
+			if (set_security_class(tdbb, rpb->rpb_record, f_rol_class))
+				DFW_post_work(transaction, dfw_grant, &desc, obj_sql_role);
+
 		case rel_db_creators:
 			if (!tdbb->getAttachment()->locksmith(tdbb, GRANT_REVOKE_ANY_DDL_RIGHT))
 				protect_system_table_insert(tdbb, request, relation);