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.

Sign up for GitHub

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

Enhancements for db_securityadmin fixed role #3344

Merged
merged 3 commits into from
Jan 3, 2025
Merged

Enhancements for db_securityadmin fixed role #3344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ba5670c
Allow GRANT/REVOKE on sequence to members of db_securityadmin
HarshLunagariya Dec 29, 2024
bea9de4
Add is_rolemember tests
HarshLunagariya Jan 2, 2025
84720e0
add comment in test
HarshLunagariya Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions contrib/babelfishpg_tsql/src/hooks.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5723,6 +5723,7 @@ handle_grantstmt_for_dbsecadmin(ObjectType objType, Oid objId, Oid ownerId,
case OBJECT_TABLE:
case OBJECT_COLUMN:
case OBJECT_VIEW:
case OBJECT_SEQUENCE:
classid = RelationRelationId;
break;
case OBJECT_FUNCTION:
Expand Down
17 changes: 11 additions & 6 deletions contrib/babelfishpg_tsql/src/rolecmds.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2050,17 +2050,22 @@ check_alter_role_stmt(GrantRoleStmt *stmt)
if (has_privs_of_role(GetUserId(), db_owner))
return;

/*
* Members of db_securityadmin role can ALTER ANY ROLE
*/
if (get_db_principal_kind(granted, db_name) == BBF_ROLE &&
has_privs_of_role(GetUserId(), get_db_securityadmin_oid(db_name, false)))
{
return;
}

/*
* Disallow ALTER ROLE if
* 1. Current login doesn't have permission on the granted role
* OR
* 2. Granted role is not a fixed db role or current user is a member of db_securityadmin
* OR
* 3. The current user is trying to add/drop itself from the granted role
* 2. The current user is trying to add/drop itself from the granted role
*/
if ((!has_privs_of_role(GetSessionUserId(), granted) &&
!(get_db_principal_kind(granted, db_name) == BBF_ROLE &&
has_privs_of_role(GetUserId(), get_db_securityadmin_oid(get_current_pltsql_db_name(), false)))) ||
if (!has_privs_of_role(GetSessionUserId(), granted) ||
grantee == GetUserId())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
Expand Down
70 changes: 70 additions & 0 deletions test/JDBC/expected/BABEL-ROLE-MEMBER.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1343,6 +1343,76 @@ GO
~~ERROR (Message: The role has members. It must be empty before it can be dropped.)~~


-- tsql
-- Test system defined database role memberships
-- Following should return 1
select is_rolemember('db_ddladmin', 'db_ddladmin')
GO
~~START~~
int
1
~~END~~

select is_rolemember('db_securityadmin', 'db_securityadmin')
GO
~~START~~
int
1
~~END~~

select is_rolemember('db_accessadmin', 'db_accessadmin')
GO
~~START~~
int
1
~~END~~

select is_rolemember('db_datareader', 'db_datareader')
GO
~~START~~
int
1
~~END~~

select is_rolemember('db_datawriter', 'db_datawriter')
GO
~~START~~
int
1
~~END~~

select is_rolemember('db_owner', 'db_owner')
GO
~~START~~
int
1
~~END~~

select is_rolemember('db_owner', 'dbo')
GO
~~START~~
int
1
~~END~~

select is_rolemember('dbo', 'dbo')
GO
~~START~~
int
1
~~END~~


-- This should return NULL
select is_rolemember('dbo', 'db_owner')
GO
~~START~~
int
<NULL>
~~END~~



-- Clean up
DROP USER test_user1
GO
Expand Down
2 changes: 2 additions & 0 deletions test/JDBC/expected/db_securityadmin-vu-cleanup.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ DROP FUNCTION babel_5135_schema1.babel_5135_f1();
GO
DROP FUNCTION babel_5135_schema1.babel_5135_tvf1();
GO
DROP SEQUENCE babel_5135_schema1.babel_5135_seq1;
GO
DROP PROCEDURE babel_5135_roleop_proc1;
GO
DROP PROCEDURE babel_5135_roleop_proc2;
Expand Down
5 changes: 5 additions & 0 deletions test/JDBC/expected/db_securityadmin-vu-prepare.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ GO
CREATE FUNCTION babel_5135_schema1.babel_5135_tvf1() RETURNS TABLE AS RETURN (SELECT a, b FROM babel_5135_schema1.babel_5135_t1);
GO

CREATE SEQUENCE babel_5135_schema1.babel_5135_seq1 START WITH 1 INCREMENT BY 1 MINVALUE 1 MAXVALUE 999999 CYCLE;
GO

CREATE VIEW babel_5135_show_role_mem AS
SELECT
roles.name AS RolePrincipalName
Expand All @@ -63,6 +66,7 @@ GRANT SELECT ON babel_5135_schema1.babel_5135_v1 TO babel_5135_u1;
GRANT EXECUTE ON babel_5135_schema1.babel_5135_p1 TO babel_5135_u1;
GRANT EXECUTE ON babel_5135_schema1.babel_5135_f1 TO babel_5135_u1;
GRANT EXECUTE ON babel_5135_schema1.babel_5135_tvf1 TO babel_5135_u1;
GRANT UPDATE ON babel_5135_schema1.babel_5135_seq1 TO babel_5135_u1;
END
GO
CREATE PROCEDURE babel_5135_revokeop_proc1 AS BEGIN
Expand All @@ -71,6 +75,7 @@ REVOKE SELECT ON babel_5135_schema1.babel_5135_v1 FROM babel_5135_u1;
REVOKE EXECUTE ON babel_5135_schema1.babel_5135_p1 FROM babel_5135_u1;
REVOKE EXECUTE ON babel_5135_schema1.babel_5135_f1 FROM babel_5135_u1;
REVOKE EXECUTE ON babel_5135_schema1.babel_5135_tvf1 FROM babel_5135_u1;
REVOKE UPDATE ON babel_5135_schema1.babel_5135_seq1 FROM babel_5135_u1;
END
GO

Expand Down
131 changes: 129 additions & 2 deletions test/JDBC/expected/db_securityadmin-vu-verify.out
View file
Open in desktop
HarshLunagariya marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,9 @@ GO
-- tsql
-- CASE 3 - Able to manage database roles
-- CASE 3.1 - CREATE/ALTER/DROP ROLE
-- CASE 3.2 - ADD/DROP the membership of user-defined database roles should be allowed
-- CASE 3.2.a - ADD/DROP the membership of user-defined database roles should be allowed
-- CASE 3.2.b - ADD/DROP the membership of user-defined database roles to/from current_user
-- CASE 3.2.c - It should be able to ALTER the membership of database role which is member of db_owner
-- CASE 3.3 - ADD/DROP the membership of system-defined database roles should be blocked
-- CASE 3.4 - CREATE/ALTER/DROP USER should not be Allowed
-- role created by another user, to test alter/drop on it
Expand Down Expand Up @@ -157,13 +159,50 @@ GO
EXEC babel_5135_roleop_proc1;
GO

-- CASE 3.2 - ADD/DROP the membership of user-defined database roles
-- CASE 3.2.a - ADD/DROP the membership of user-defined database roles
ALTER ROLE babel_5135_r1 ADD MEMBER babel_5135_u1;
GO

ALTER ROLE babel_5135_r1 DROP MEMBER babel_5135_u1;
GO

-- CASE 3.2.b - ADD/DROP the membership of user-defined database roles to/from current_user
SELECT current_user;
GO
~~START~~
varchar
babel_5135_dbsecadmin_u1
~~END~~


ALTER ROLE babel_5135_r1 ADD MEMBER babel_5135_dbsecadmin_u1;
GO

ALTER ROLE babel_5135_r1 DROP MEMBER babel_5135_dbsecadmin_u1;
GO

-- CASE 3.2.c - It should be able to ALTER the membership of database role which is member of db_owner
-- Currently it is not supported to add database role to db_owner
ALTER ROLE db_owner ADD MEMBER babel_5135_r1;
GO
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Adding database roles to db_owner is not currently supported in Babelfish)~~
Comment on lines +186 to +190
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment above says it should be able ALTER the membership but here we are getting an error. Is this error expected? If yes then we should chose a different test case in which current user is able to ALTER the membership of db_owner.

Copy link
Contributor Author

@HarshLunagariya HarshLunagariya Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is added for future when we add support of adding database roles as member of db_owner. Will add a comment regarding it.



ALTER ROLE babel_5135_r1 ADD MEMBER babel_5135_dbsecadmin_u1;
GO

ALTER ROLE babel_5135_r1 DROP MEMBER babel_5135_dbsecadmin_u1;
GO
Comment on lines +193 to +197
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this test repeated? It is already there at line number 178 and 181.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this scenario, we wanted to test whether we can add current user as member of babel_5135_r1 (when it is member of db_owner).


ALTER ROLE db_owner DROP MEMBER babel_5135_r1;
GO
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Dropping database roles from db_owner is not currently supported in Babelfish)~~
Comment on lines +199 to +203
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, I don't think we intend to test this error in this scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is added for future when we add support of adding database roles as member of db_owner. Will add a comment regarding it.



-- alter role add member inside procedure
-- execution should be succeeded with no error
-- Add
Expand All @@ -189,6 +228,20 @@ GO
~~ERROR (Message: Cannot alter the role 'db_owner', because it does not exist or you do not have permission.)~~


ALTER ROLE babel_5135_r1 DROP MEMBER dbo;
GO
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot use the special principal 'dbo')~~


ALTER ROLE babel_5135_r1 ADD MEMBER dbo;
GO
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot use the special principal 'dbo')~~


-- CASE 3.4 -- CREATE/ALTER/DROP USER should fail
CREATE USER babel_5135_user1 FOR LOGIN babel_5135_l2;
GO
Expand Down Expand Up @@ -345,6 +398,13 @@ GO
~~ERROR (Message: permission denied for function babel_5135_tvf1)~~


GRANT UPDATE ON babel_5135_schema1.babel_5135_seq1 TO babel_5135_u1;
GO
~~ERROR (Code: 33557097)~~

~~ERROR (Message: permission denied for sequence babel_5135_seq1)~~


-- tsql user=babel_5135_l1 password=12345678
SELECT current_user;
GO
Expand Down Expand Up @@ -392,6 +452,15 @@ GO
~~ERROR (Message: permission denied for function babel_5135_tvf1)~~


SELECT NEXT VALUE FOR babel_5135_schema1.babel_5135_seq1;
GO
~~START~~
bigint
~~ERROR (Code: 33557097)~~

~~ERROR (Message: permission denied for sequence babel_5135_seq1)~~


-- tsql user=babel_5135_dbsecadmin_l1 password=12345678
REVOKE SELECT, INSERT, UPDATE, DELETE ON babel_5135_schema1.babel_5135_t1 FROM babel_5135_u1;
GO
Expand Down Expand Up @@ -455,6 +524,15 @@ GO
~~ERROR (Message: permission denied for function babel_5135_tvf1)~~


SELECT NEXT VALUE FOR babel_5135_schema1.babel_5135_seq1;
GO
~~START~~
bigint
~~ERROR (Code: 33557097)~~

~~ERROR (Message: permission denied for sequence babel_5135_seq1)~~


-- tsql user=babel_5135_dbsecadmin_l1 password=12345678
-- Testing GRANT inside procedure
EXEC babel_5135_grantop_proc1;
Expand Down Expand Up @@ -517,6 +595,14 @@ int#!#int
~~END~~


SELECT NEXT VALUE FOR babel_5135_schema1.babel_5135_seq1;
GO
~~START~~
bigint
1
~~END~~


-- tsql user=babel_5135_dbsecadmin_l1 password=12345678
-- Testing revokes inside procedure
EXEC babel_5135_revokeop_proc1;
Expand Down Expand Up @@ -569,6 +655,14 @@ GO
~~ERROR (Message: permission denied for function babel_5135_tvf1)~~


SELECT NEXT VALUE FOR babel_5135_schema1.babel_5135_seq1;
GO
~~START~~
bigint
~~ERROR (Code: 33557097)~~

~~ERROR (Message: permission denied for sequence babel_5135_seq1)~~


-- tsql user=babel_5135_dbsecadmin_l1 password=12345678
GRANT SELECT, INSERT, UPDATE, DELETE, EXECUTE ON SCHEMA::babel_5135_schema1 TO babel_5135_u1;
Expand Down Expand Up @@ -631,6 +725,15 @@ int#!#int
~~END~~


SELECT NEXT VALUE FOR babel_5135_schema1.babel_5135_seq1;
GO
~~START~~
bigint
~~ERROR (Code: 33557097)~~

~~ERROR (Message: permission denied for sequence babel_5135_seq1)~~


-- tsql user=babel_5135_dbsecadmin_l1 password=12345678
REVOKE SELECT, INSERT, UPDATE, DELETE, EXECUTE ON SCHEMA::babel_5135_schema1 FROM babel_5135_u1;
GO
Expand Down Expand Up @@ -682,6 +785,15 @@ GO
~~ERROR (Message: permission denied for function babel_5135_tvf1)~~


SELECT NEXT VALUE FOR babel_5135_schema1.babel_5135_seq1;
GO
~~START~~
bigint
~~ERROR (Code: 33557097)~~

~~ERROR (Message: permission denied for sequence babel_5135_seq1)~~


-- tsql user=babel_5135_dbsecadmin_l1 password=12345678
-- CASE 5.2 - Validate members of db_securityadmin can not actually access given objects
SELECT current_user;
Expand Down Expand Up @@ -730,6 +842,15 @@ GO
~~ERROR (Message: permission denied for function babel_5135_tvf1)~~


SELECT NEXT VALUE FOR babel_5135_schema1.babel_5135_seq1;
GO
~~START~~
bigint
~~ERROR (Code: 33557097)~~

~~ERROR (Message: permission denied for sequence babel_5135_seq1)~~


-- tsql user=babel_5135_dbsecadmin_l1 password=12345678
-- CASE 5.3 - Validate that after GRANT/REVOKE, objectowner/dbo can execute REVOKE/GRANT respectively
-- execute GRANT via db_securityadmin member and REVOKE it with object owner
Expand All @@ -748,6 +869,9 @@ GO
GRANT EXECUTE ON babel_5135_schema1.babel_5135_tvf1 TO babel_5135_u1;
GO

GRANT UPDATE ON babel_5135_schema1.babel_5135_seq1 TO babel_5135_u1;
GO

-- tsql
REVOKE SELECT, INSERT, UPDATE, DELETE ON babel_5135_schema1.babel_5135_t1 FROM babel_5135_u1;
GO
Expand Down Expand Up @@ -780,6 +904,9 @@ GO
GRANT EXECUTE ON babel_5135_schema1.babel_5135_tvf1 TO babel_5135_u1;
GO

GRANT UPDATE ON babel_5135_schema1.babel_5135_seq1 TO babel_5135_u1;
GO

-- tsql user=babel_5135_dbsecadmin_l1 password=12345678
REVOKE SELECT, INSERT, UPDATE, DELETE ON babel_5135_schema1.babel_5135_t1 FROM babel_5135_u1;
GO
Expand Down
Loading