Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support to sys.server_role_members #1722

Merged

Conversation

anju15bharti
Copy link
Contributor

@anju15bharti anju15bharti commented Aug 10, 2023

Description

Currently, Babelfish does not support TSQL's sys.server_role_members view which is needed to determine the role(sysadmin server role) membership status. This commit implements this view and provides testing.

Task: BABEL-2532
Authored-by: Anju Bharti [email protected]
Signed-off-by: Anju Bharti [email protected]

Issues Resolved
JIRA BABEL-2532
view sys.server_role_members was not implemented

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is under the terms of the Apache 2.0 and PostgreSQL licenses, and grant any person obtaining a copy of the contribution permission to relicense all or a portion of my contribution to the PostgreSQL License solely to contribute all or a portion of my contribution to the PostgreSQL open source project.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Comment on lines 378 to 388
CREATE OR REPLACE VIEW sys.server_role_members AS
SELECT
CAST(Authmbr.roleid AS INT) AS role_principal_id,
CAST(Authmbr.member AS INT) AS member_principal_id
FROM pg_catalog.pg_auth_members AS Authmbr
INNER JOIN sys.server_principals AS p1 ON p1.principal_id = Authmbr.roleid
INNER JOIN sys.server_principals AS p2 ON p2.principal_id = Authmbr.member
WHERE p1.type_desc='SERVER_ROLE'
AND p1.is_fixed_role=1;

GRANT SELECT ON sys.server_role_members TO PUBLIC;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be targeted for 3.4.0 version. So, please rebase it once 3.x development branch is ready for 3.4.0.

contrib/babelfishpg_tsql/sql/ownership.sql Outdated Show resolved Hide resolved
Comment on lines 1 to 17
SELECT * from sys_server_role_members_vu_prepare_view;
GO

ALTER SERVER ROLE sysadmin ADD MEMBER sys_server_role_members_vu_prepare_login1;
GO

ALTER SERVER ROLE sysadmin ADD MEMBER sys_server_role_members_vu_prepare_login2;
GO

SELECT * from sys_server_role_members_vu_prepare_view;
GO

ALTER SERVER ROLE sysadmin DROP MEMBER sys_server_role_members_vu_prepare_login2;
GO

ALTER SERVER ROLE sysadmin ADD MEMBER sys_server_role_members_vu_prepare_login3;
GO
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we need to test visibility according to the current_login. There may be a case that this view can only show data about current login, not others unless it has some sysadmin - we need to verify such scenarios.

Please check the .mix files for other test - We can also use multiple connections with specific login & database.

test/JDBC/expected/sys_server_role_members-vu-prepare.out Outdated Show resolved Hide resolved
@HarshLunagariya HarshLunagariya self-requested a review October 25, 2023 03:21
Comment on lines +489 to +493
INNER JOIN pg_catalog.pg_roles AS Auth1 ON Auth1.oid = Authmbr.roleid
INNER JOIN pg_catalog.pg_roles AS Auth2 ON Auth2.oid = Authmbr.member
INNER JOIN sys.babelfish_authid_login_ext AS Ext1 ON Auth1.rolname = Ext1.rolname
INNER JOIN sys.babelfish_authid_login_ext AS Ext2 ON Auth2.rolname = Ext2.rolname
WHERE Ext1.type = 'R';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the permissions? Should all server membership should be visible by default? Please refer doc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Babelfish, we only allow one server role sysadmin which is marked as "R" in the catalog.
In SQL Server, the default server role sysadmin is visible to everyone. There is no behavioral difference.
We don't support creating a new server role other that 'sysadmin' in Babelfish, do we?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, In future when we support it at that time we may miss it. It will be better to add a condition to check that if it is fixed server role or showing membership's for current server principal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.
Currently, we hardcode is_fixed_role to 0 always.
I have suggested a change to Anju on how to update the is_fixed_role column in babelfish_authid_login_ext catalog which will be helpful for #1948 as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need to update sys.server_role_members view once we support create server role syntax in Babelfish as tracked by https://jira.rds.a2z.com/browse/BABEL-113

@shalinilohia50 shalinilohia50 self-requested a review October 25, 2023 09:17
@shardgupta shardgupta merged commit 7c0e468 into babelfish-for-postgresql:BABEL_3_X_DEV Oct 26, 2023
27 checks passed
@shardgupta shardgupta deleted the JIRA-BABEL-2532 branch October 26, 2023 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants