[OSS-ONLY] Login with mapped user should not use guest user privileges (

#3088) (#3156)

Previously, a guest users always remain member of a login even if login has a mapped
user in a particular database. Due to this any database user is able to access the
objects which are accessible to guest user which is undesirable.

Fixed this issue by only keeping one user member of a login at a time, which means,
guest will be a member of login only if there is no mapped user to that login otherwise
only that mapped user will be a member of login.

Task: BABEL-5389

Co-authored-by: Rishabh Tanwar <[email protected]>
Signed-off-by: Shalini Lohia <[email protected]>

contrib/babelfishpg_tsql/sql/upgrades/babelfishpg_tsql--4.3.0--4.4.0.sql

@@ -37,6 +37,17 @@ LANGUAGE plpgsql;
  * So make sure that any SQL statement (DDL/DML) being added here can be executed multiple times without affecting
  * final behaviour.
  */
+
+-- This is a temporary procedure which is only meant to be called during upgrade
+CREATE OR REPLACE PROCEDURE sys.babelfish_revoke_guest_from_mapped_logins()
+LANGUAGE C
+AS 'babelfishpg_tsql', 'revoke_guest_from_mapped_logins';
+
+CALL sys.babelfish_revoke_guest_from_mapped_logins();
+
+-- Drop this procedure after it gets executed once.
+DROP PROCEDURE sys.babelfish_revoke_guest_from_mapped_logins();
+
 DO $$
 DECLARE
     exception_message text;

contrib/babelfishpg_tsql/src/catalog.c

@@ -943,10 +943,10 @@ get_authid_user_ext_physical_name(const char *db_name, const char *login)
 {
 	Relation	bbf_authid_user_ext_rel;
 	HeapTuple	tuple_user_ext;
-	ScanKeyData key[3];
 	TableScanDesc scan;
 	char	   *user_name = NULL;
 	NameData   *login_name;
+	ScanKeyData key[2];
 
 	if (!db_name || !login)
 		return NULL;
@@ -956,20 +956,16 @@ get_authid_user_ext_physical_name(const char *db_name, const char *login)
 
 	login_name = (NameData *) palloc0(NAMEDATALEN);
 	snprintf(login_name->data, NAMEDATALEN, "%s", login);
+
 	ScanKeyInit(&key[0],
-				Anum_bbf_authid_user_ext_login_name,
-				BTEqualStrategyNumber, F_NAMEEQ,
-				NameGetDatum(login_name));
+			Anum_bbf_authid_user_ext_login_name,
+			BTEqualStrategyNumber, F_NAMEEQ,
+			NameGetDatum(login_name));
 	ScanKeyInit(&key[1],
-				Anum_bbf_authid_user_ext_database_name,
-				BTEqualStrategyNumber, F_TEXTEQ,
-				CStringGetTextDatum(db_name));
-	ScanKeyInit(&key[2],
-				Anum_bbf_authid_user_ext_user_can_connect,
-				BTEqualStrategyNumber, F_INT4EQ,
-				Int32GetDatum(1));
-
-	scan = table_beginscan_catalog(bbf_authid_user_ext_rel, 3, key);
+			Anum_bbf_authid_user_ext_database_name,
+			BTEqualStrategyNumber, F_TEXTEQ,
+			CStringGetTextDatum(db_name));
+	scan = table_beginscan_catalog(bbf_authid_user_ext_rel, 2, key);
 
 	tuple_user_ext = heap_getnext(scan, ForwardScanDirection);
 	if (HeapTupleIsValid(tuple_user_ext))
@@ -1074,6 +1070,28 @@ get_authid_user_ext_db_users(const char *db_name)
 	return db_users_list;
 }
 
+/* Checks if the user is enabled on a given database. */
+static bool
+user_has_dbaccess(const char *user)
+{
+	HeapTuple	tuple;
+	bool		has_access = false;
+	tuple = SearchSysCache1(AUTHIDUSEREXTROLENAME, CStringGetDatum(user));
+
+	if (HeapTupleIsValid(tuple))
+	{
+		bool	isnull = true;
+		int	user_can_connect = 0;
+		Datum	datum = SysCacheGetAttr(AUTHIDUSEREXTROLENAME, tuple, Anum_bbf_authid_user_ext_user_can_connect, &isnull);
+		Assert(!isnull);
+		user_can_connect = DatumGetInt32(datum);
+		if (user_can_connect == 1)
+			has_access = true;
+		ReleaseSysCache(tuple);
+	}
+	return has_access;
+}
+
 /*
  * Checks if there exists any user for respective database and login,
  * if there is not any then use dbo or guest user.
@@ -1091,6 +1109,9 @@ get_user_for_database(const char *db_name)
 	user = get_authid_user_ext_physical_name(db_name, login);
 	login_is_db_owner = 0 == strncmp(login, get_owner_of_db(db_name), NAMEDATALEN);
 
+	if (user && !user_has_dbaccess(user) && !guest_has_dbaccess((char *) db_name))
+		user = NULL;
+
 	if (!user)
 	{
 		Oid			datdba;
@@ -3139,7 +3160,7 @@ create_guest_role_for_db(const char *dbname)
 	if (list_length(logins) > 0)
 	{
 		stmt = parsetree_nth_stmt(res, i++);
-		update_GrantRoleStmt(stmt, list_make1(make_accesspriv_node(guest)), logins);
+		update_GrantRoleStmt(stmt, list_make1(make_accesspriv_node(guest)), logins, NULL);
 	}
 
 	GetUserIdAndSecContext(&save_userid, &save_sec_context);

contrib/babelfishpg_tsql/src/dbcmds.c

@@ -167,7 +167,7 @@ gen_createdb_subcmds(const char *dbname, const char *owner)
 		/* Grant dbo role to owner */
 		stmt = parsetree_nth_stmt(res, i++);
 		update_GrantRoleStmt(stmt, list_make1(make_accesspriv_node(dbo)),
-							list_make1(make_rolespec_node(owner)));
+							list_make1(make_rolespec_node(owner)), NULL);
 	}
 
 	if (guest)
@@ -178,7 +178,7 @@ gen_createdb_subcmds(const char *dbname, const char *owner)
 		if (list_length(logins) > 0)
 		{
 			stmt = parsetree_nth_stmt(res, i++);
-			update_GrantRoleStmt(stmt, list_make1(make_accesspriv_node(guest)), logins);
+			update_GrantRoleStmt(stmt, list_make1(make_accesspriv_node(guest)), logins, NULL);
 		}
 	}
 

contrib/babelfishpg_tsql/src/pl_exec-2.c

@@ -3908,10 +3908,10 @@ exec_stmt_change_dbowner(PLtsql_execstate *estate, PLtsql_stmt_change_dbowner *s
 		SetUserIdAndSecContext(get_bbf_role_admin_oid(), save_sec_context | SECURITY_LOCAL_USERID_CHANGE);
 
 		/* Revoke dbo role from the previous owner */
-		grant_revoke_dbo_to_login(get_owner_of_db(stmt->db_name), stmt->db_name, false);
+		grant_revoke_role_to_login(get_owner_of_db(stmt->db_name), get_dbo_role_name(stmt->db_name), NULL, false);
 
 		/* Grant dbo role to the new owner */
-		grant_revoke_dbo_to_login(stmt->new_owner_name, stmt->db_name, true);
+		grant_revoke_role_to_login(stmt->new_owner_name, get_dbo_role_name(stmt->db_name), NULL, true);
 		update_db_owner(stmt->new_owner_name, stmt->db_name);	
 	}
 	PG_FINALLY();

contrib/babelfishpg_tsql/src/pltsql.h

@@ -2168,7 +2168,7 @@ extern void update_CreateSchemaStmt(Node *n, const char *schemaname, const char
 extern void update_DropOwnedStmt(Node *n, List *role_list);
 extern void update_DropRoleStmt(Node *n, const char *role);
 extern void update_DropStmt(Node *n, const char *object);
-extern void update_GrantRoleStmt(Node *n, List *privs, List *roles);
+extern void update_GrantRoleStmt(Node *n, List *privs, List *roles, const char *grantor);
 extern void update_GrantStmt(Node *n, const char *object, const char *obj_schema, const char *grantee, const char *priv);
 extern void update_RenameStmt(Node *n, const char *old_name, const char *new_name);
 extern void update_ViewStmt(Node *n, const char *view_schema);

contrib/babelfishpg_tsql/src/pltsql_utils.c

@@ -1082,7 +1082,7 @@ update_DropStmt(Node *n, const char *object)
 }
 
 void
-update_GrantRoleStmt(Node *n, List *privs, List *roles)
+update_GrantRoleStmt(Node *n, List *privs, List *roles, const char *grantor)
 {
 	GrantRoleStmt *stmt = (GrantRoleStmt *) n;
 
@@ -1091,6 +1091,11 @@ update_GrantRoleStmt(Node *n, List *privs, List *roles)
 
 	stmt->granted_roles = privs;
 	stmt->grantee_roles = roles;
+
+	if (grantor && stmt->grantor)
+	{
+		stmt->grantor->rolename = pstrdup(grantor);
+	}
 }
 
 void