Added support for sys.server_permissions and sys.sql_logins views. #3538

shreyeah38 · 2025-02-28T05:11:48Z

Description

This PR adds full support for sys.server_permissions and sys.sql_logins views, addressing a current limitation in Babelfish. The implementation includes essential system views sys.server_permissions and sys.sql_logins required for comprehensive login export capabilities. This enhancement enables users to view server permissions and details about the logins in Babelfish, maintaining compatibility with TSQL Server syntax.

Added Components

Views:

sys.server_permissions: For managing server-level permissions
sys.sql_logins: For managing SQL Server authentication logins

BABEL-5654

Signed-off-by: Shreya Rai [email protected]

Signed-off-by: Shreya Rai <[email protected]>

coveralls · 2025-02-28T15:59:22Z

Pull Request Test Coverage Report for Build 13915600009

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
729 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.009%) to 75.034%

Files with Coverage Reduction	New Missed Lines	%
contrib/babelfishpg_tsql/src/tsql_for/forjson.c	18	90.49%
contrib/babelfishpg_tsql/src/collation.c	147	72.9%
contrib/babelfishpg_tsql/src/pl_handler.c	212	90.34%
contrib/babelfishpg_tsql/src/tsqlIface.cpp	352	90.14%

Totals
Change from base Build 13793050061:	0.009%
Covered Lines:	47373
Relevant Lines:	63135

💛 - Coveralls

Signed-off-by: Shreya Rai <[email protected]>

shalinilohia50 · 2025-03-04T08:34:28Z

contrib/babelfishpg_tsql/sql/sys_views.sql

+            WHERE datname = CURRENT_DATABASE() COLLATE sys.database_default
+        ) 
+     ) 
+CROSS JOIN 


These joins could be simplified as suggested offline.

Okay, I will fix this in next commit.

Signed-off-by: Shreya Rai <[email protected]>

shalinilohia50 · 2025-03-05T08:22:57Z

test/JDBC/expected/sys_server_permissions-vu-prepare.out

@@ -1,18 +1,22 @@
-USE master
+-- tsql
+CREATE LOGIN sys_server_permissions_vu_login1 with password = '123'


Add tests with window login.

CREATE LOGIN [ad\Aduser] from windows;

Yes, adding.

shalinilohia50 · 2025-03-05T08:24:20Z

contrib/babelfishpg_tsql/sql/upgrades/babelfishpg_tsql--5.1.0--5.2.0.sql

+  OR pg_has_role(sys.suser_id(), 'securityadmin'::TEXT, 'MEMBER')
+  OR Ext.orig_loginname = sys.suser_name() 
+  OR Ext.orig_loginname = (SELECT pg_get_userbyid(datdba) FROM pg_database WHERE datname = CURRENT_DATABASE()) COLLATE sys.database_default) 
+  AND Ext.type IN ('S');


AND Ext.type = 'S';

Yes, I'll make these changes

Signed-off-by: Shreya Rai <[email protected]>

shalinilohia50 · 2025-03-06T04:07:55Z

contrib/babelfishpg_tsql/sql/sys_views.sql

+WHERE (pg_has_role(sys.suser_id(), 'sysadmin'::TEXT, 'MEMBER')
+    OR pg_has_role(sys.suser_id(), 'securityadmin'::TEXT, 'MEMBER')
+    OR Base.rolname = sys.suser_name() COLLATE sys.database_default 
+    OR Base.rolname = Master.rolname)


OR Base.rolname = Master.rolname)

This means when grantee and grantor are same. Why is this check needed? Which case do we cover with this check?

CREATE OR REPLACE VIEW sys.server_permissions AS SELECT CAST(100 AS sys.tinyint) AS class, CAST('SERVER' AS sys.nvarchar(60)) AS class_desc, CAST(0 AS int) AS major_id, CAST(0 AS int) AS minor_id, CAST(Base.oid AS INT) AS grantee_principal_id, CAST((SELECT oid FROM pg_catalog.pg_roles WHERE rolname = (SELECT pg_get_userbyid(datdba) FROM pg_database WHERE datname = CURRENT_DATABASE()) ) AS INT) AS grantor_principal_id, CAST('COSQ' AS sys.BPCHAR(4)) AS type, CAST('CONNECT SQL' AS sys.nvarchar(128)) AS permission_name, CAST('G' AS sys.BPCHAR(1)) AS state, CAST('GRANT' AS sys.nvarchar(60)) AS state_desc FROM pg_catalog.pg_roles AS Base INNER JOIN sys.babelfish_authid_login_ext AS Ext ON Base.rolname = Ext.rolname WHERE (pg_has_role(sys.suser_id(), 'sysadmin'::TEXT, 'MEMBER') OR pg_has_role(sys.suser_id(), 'securityadmin'::TEXT, 'MEMBER') OR Base.rolname = sys.suser_name() COLLATE sys.database_default OR Base.rolname = (SELECT pg_get_userbyid(datdba) FROM pg_database WHERE datname = CURRENT_DATABASE()))

I think we can get rid of an INNER JOIN for Grantor information.

Yes, fixed that.

shalinilohia50 · 2025-03-06T04:37:14Z

test/JDBC/expected/sys_sql_logins-vu-verify.out

+
+-- tsql user=sys_sql_logins_vu_login1 password=123
+SELECT * FROM dbo.sys_sql_logins_vu_prepare_view
+GO


Please add tests to check other columns of the view like is_policy_checked, is_expiration_checked etc.

shalinilohia50 · 2025-03-06T04:39:56Z

test/JDBC/expected/sys_server_permissions-vu-verify.out

+SELECT * FROM sys_server_permissions_vu_prepare_view
+GO
+~~START~~
+varchar#!#nvarchar#!#nvarchar#!#nvarchar


Add tests to cover all the columns in the view.

Added few more columns in the test files.

shalinilohia50 · 2025-03-06T04:45:12Z

contrib/babelfishpg_tsql/sql/sys_views.sql

+    CASE
+      WHEN Ext.type = 'S' THEN 'SQL_LOGIN' 
+      WHEN Ext.type = 'R' THEN 'SERVER_ROLE' 
+      WHEN Ext.type = 'U' THEN 'WINDOWS_LOGIN' 
+      ELSE NULL 
+    END 


We have a condition below AND Ext.type = 'S', so these cases are invalid. type_desc can only be SQL_LOGIN

Yes, fixed it.

Signed-off-by: Shreya Rai <[email protected]>

anju15bharti · 2025-03-06T08:37:28Z

test/JDBC/expected/sys_sql_logins-vu-verify.out

@@ -1,31 +1,99 @@
-USE master


Reset the passoward of logins at the top.

anju15bharti · 2025-03-06T08:42:23Z

contrib/babelfishpg_tsql/sql/sys_views.sql

+AS 
+SELECT 
+   CAST(100 AS sys.tinyint) AS class,
+   CAST('SERVER' AS sys.nvarchar(60)) AS class_desc,


Is the value of class_desc is SERVER for windows logins as well?

anju15bharti · 2025-03-06T08:47:06Z

contrib/babelfishpg_tsql/sql/sys_views.sql

+  CAST(Ext.credential_id AS INT) AS credential_id,
+  CAST(
+    CASE 
+      WHEN Ext.orig_loginname = (SELECT pg_get_userbyid(datdba) FROM pg_database WHERE datname = CURRENT_DATABASE()) COLLATE sys.database_default THEN 0


repetitive call of SELECT pg_get_userbyid(datdba) FROM pg_database WHERE datname = CURRENT_DATABASE()) optimize it.

Made the required changes.

anju15bharti · 2025-03-06T08:57:50Z

test/JDBC/input/views/sys_sql_logins-vu-verify.mix

@@ -0,0 +1,51 @@
+-- tsql


Add cases for scenarios where login is member of multiple fixed server roles for both the testfiles.

Add negative testcases to show server roles are not being displayed in sql_logins view.

Signed-off-by: Shreya Rai <[email protected]>

anju15bharti · 2025-03-17T04:59:54Z

contrib/babelfishpg_tsql/sql/sys_views.sql

+    ELSE NULL
+  END
+AS sys.varbinary(256)) AS password_hash 
+FROM sys.server_principals AS Base 


from performance pov I think we should not fetch columns from view server_principals rather it should be fetched from base catalog itself. Let's get @HarshLunagariya's opinion on this.

Can you please provide the execution plan for both variations for analyze?

HarshLunagariya · 2025-03-17T05:57:50Z

contrib/babelfishpg_tsql/sql/sys_views.sql

+CREATE OR REPLACE VIEW sys.server_permissions AS 
+WITH super_user AS (SELECT datdba AS super_user FROM pg_database WHERE datname = CURRENT_DATABASE()) 
+SELECT 
+CAST(100 AS sys.tinyint) AS class,


When does it return 101 class id server permission?

100 = Server, 101 = Server-principal, 105 = Endpoint, 108 = Availability Group

HarshLunagariya · 2025-03-17T07:48:38Z

contrib/babelfishpg_tsql/sql/sys_views.sql

+CAST('COSQ' AS sys.BPCHAR(4)) AS type,
+CAST('CONNECT SQL' AS sys.nvarchar(128)) AS permission_name,
+CAST('G' AS sys.BPCHAR(1)) AS state,
+CAST('GRANT' AS sys.nvarchar(60)) AS state_desc 


For super_user, Shouldn't it show GRANT_WITH_GRANT_OPTION ? Since super_user can grant this privilege to others as per ideal T-SQL behaviour, Although we don't support it granting.

I compared it against TSQL behaviour, for super_user, it shows GRANT permission only.

HarshLunagariya · 2025-03-17T07:50:25Z

contrib/babelfishpg_tsql/sql/sys_views.sql

+WHERE(pg_has_role(sys.suser_id(), 'sysadmin'::TEXT, 'MEMBER')
+  OR pg_has_role(sys.suser_id(), 'securityadmin'::TEXT, 'MEMBER')
+  OR Base.rolname = sys.suser_name() COLLATE sys.database_default 
+  OR Base.rolname = (SELECT pg_get_userbyid(super_user) FROM super_user))


Do we really need OR Base.rolname = (SELECT pg_get_userbyid(super_user) FROM super_user)) , if we have (pg_has_role(sys.suser_id(), 'sysadmin'::TEXT, 'MEMBER') already? Won't it cover super_user as well?

No it doesn't, we have to use Base.rolname = (SELECT pg_get_userbyid(super_user) FROM super_user)

HarshLunagariya · 2025-03-17T07:55:53Z

contrib/babelfishpg_tsql/sql/sys_views.sql

+CAST('CO' AS sys.BPCHAR(4)) AS type,
+CAST('CONNECT' AS sys.nvarchar(128)) AS permission_name,
+CAST('G' AS sys.BPCHAR(1)) AS state,
+CAST('GRANT' AS sys.nvarchar(60)) AS state_desc;
 GRANT SELECT ON sys.server_permissions TO PUBLIC;


Are there any other permission which needs to be supported? have we analysed all of them if they are supported in Babelfish?

Since GRANT permission is not supported in Babelfish, I have only incorporated the default permissions which exist by default i.e. CONNECT SQL for each SQL and WINDOW logins and CONNECT for public via TSQL Default TCP endpoint.

HarshLunagariya · 2025-03-17T07:59:53Z

contrib/babelfishpg_tsql/sql/sys_views.sql

+WHERE(pg_has_role(sys.suser_id(), 'sysadmin'::TEXT, 'MEMBER')
+  OR pg_has_role(sys.suser_id(), 'securityadmin'::TEXT, 'MEMBER')
+  OR Base.name = sys.suser_name() 
+  OR Base.name = (SELECT super_user FROM super_user))


Same as

Do we really need OR Base.rolname = (SELECT pg_get_userbyid(super_user) FROM super_user)) , if we have (pg_has_role(sys.suser_id(), 'sysadmin'::TEXT, 'MEMBER') already? Won't it cover super_user as well?

HarshLunagariya · 2025-03-17T08:05:19Z

contrib/babelfishpg_tsql/sql/sys_views.sql

+CAST(
+  CASE
+    WHEN Base.name = (SELECT super_user FROM super_user) THEN 0
+    ELSE 1
+  END
+AS sys.BIT) AS is_policy_checked,


Just asking out of curiosity, Why does is_policy_checked column return 0 for superuser, Although password is checked for superuser as well.

It is as per TSQL, is_policy_checked column returns 0 for superuser.

HarshLunagariya · 2025-03-17T08:08:01Z

contrib/babelfishpg_tsql/sql/sys_views.sql

+    ELSE NULL
+  END
+AS sys.varbinary(256)) AS password_hash 
+FROM sys.server_principals AS Base 


Can you please provide the execution plan for both variations for analyze?

Signed-off-by: Shreya Rai <[email protected]>

shreyaxr3 added 3 commits February 28, 2025 05:02

Supporting views for logins exports

c19c989

Signed-off-by: Shreya Rai <[email protected]>

included

3d27f33

Signed-off-by: Shreya Rai <[email protected]>

changed login names

59861ff

Signed-off-by: Shreya Rai <[email protected]>

shreyaxr3 added 3 commits February 28, 2025 16:39

filtered logins

500b674

Signed-off-by: Shreya Rai <[email protected]>

fixed logins

51940af

Signed-off-by: Shreya Rai <[email protected]>

removed extra columns from sql logins

3fd0bb1

Signed-off-by: Shreya Rai <[email protected]>

shalinilohia50 reviewed Mar 4, 2025

View reviewed changes

optimised views

c900364

Signed-off-by: Shreya Rai <[email protected]>

shalinilohia50 reviewed Mar 5, 2025

View reviewed changes

shreyaxr3 added 3 commits March 5, 2025 09:32

added window logins

af09ca6

Signed-off-by: Shreya Rai <[email protected]>

fixed window login names

eb0b1a6

Signed-off-by: Shreya Rai <[email protected]>

merge conflict resolved

ed7c77b

Signed-off-by: Shreya Rai <[email protected]>

shalinilohia50 reviewed Mar 6, 2025

View reviewed changes

shreyaxr3 added 2 commits March 6, 2025 07:04

modified testfiles

521c3b2

Signed-off-by: Shreya Rai <[email protected]>

modified testfiles

3865ac4

Signed-off-by: Shreya Rai <[email protected]>

anju15bharti reviewed Mar 6, 2025

View reviewed changes

anju15bharti suggested changes Mar 6, 2025

View reviewed changes

shreyaxr3 added 8 commits March 6, 2025 09:11

fixed dotnet failure

2b78301

Signed-off-by: Shreya Rai <[email protected]>

made suggested changes

753964d

Signed-off-by: Shreya Rai <[email protected]>

resolved merge conflict

8a393d6

Signed-off-by: Shreya Rai <[email protected]>

fixed testfiles

b597b8c

Signed-off-by: Shreya Rai <[email protected]>

fmade suggested changes

5f263b7

Signed-off-by: Shreya Rai <[email protected]>

resolved merge conflict

d70bb85

Signed-off-by: Shreya Rai <[email protected]>

added endpoints

28497b8

Signed-off-by: Shreya Rai <[email protected]>

removed extra columns

36ea30a

Signed-off-by: Shreya Rai <[email protected]>

anju15bharti reviewed Mar 17, 2025

View reviewed changes

HarshLunagariya suggested changes Mar 17, 2025

View reviewed changes

made changes in sql_logins

0e2b835

Signed-off-by: Shreya Rai <[email protected]>

shalinilohia50 approved these changes Mar 18, 2025

View reviewed changes

anju15bharti approved these changes Mar 24, 2025

View reviewed changes

Added support for sys.server_permissions and sys.sql_logins views. #3538

Are you sure you want to change the base?

Added support for sys.server_permissions and sys.sql_logins views. #3538

Conversation

shreyeah38 commented Feb 28, 2025

Description

coveralls commented Feb 28, 2025 • edited Loading

Pull Request Test Coverage Report for Build 13915600009

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

shalinilohia50 Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

shreyeah38 Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shalinilohia50 Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shalinilohia50 Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anju15bharti Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreyeah38 Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreyeah38 Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 28, 2025 •

edited

Loading

shalinilohia50 Mar 4, 2025 •

edited

Loading

shreyeah38 Mar 4, 2025 •

edited

Loading

shalinilohia50 Mar 6, 2025 •

edited

Loading

shalinilohia50 Mar 6, 2025 •

edited

Loading

anju15bharti Mar 6, 2025 •

edited

Loading

shreyeah38 Mar 17, 2025 •

edited

Loading

shreyeah38 Mar 17, 2025 •

edited

Loading