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

Restrict DROP USER/ROLE from non-dbo user #2859

Merged
merged 8 commits into from
Aug 19, 2024
Merged

Restrict DROP USER/ROLE from non-dbo user #2859

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b946273
Restrict DROP USER/ROLE from non-dbo user
Aug 14, 2024
ec5d2b5
Restrict dropping of dbo and db_owner
Aug 15, 2024
149ad22
Updated BABEL-3844-vu-cleanup expected file
Aug 16, 2024
799df76
Merge remote-tracking branch 'upstream/BABEL_4_X_DEV' into jira-BABEL…
Aug 16, 2024
675f72e
Replaced physical rolename with logical rolename in error message
Aug 16, 2024
4330cdd
Added more testcases
Aug 16, 2024
64538e6
Address comments
HarshLunagariya Aug 19, 2024
918f087
Address more comments
HarshLunagariya Aug 19, 2024
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
31 changes: 30 additions & 1 deletion contrib/babelfishpg_tsql/src/pl_handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3285,15 +3285,44 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
{
RoleSpec *rolspec = lfirst(item);
char *user_name;
char *db_principal;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should pfree db_principal. db_owner_name is a const char*, it can't be freed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should not pfree db_principal as it is pointing to string literal.

Copy link
Contributor

Choose a reason for hiding this comment

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

it is it pointing to string literal then make it const char * here itself.

const char * db_principal = drop_user ? "user" : "role"

const char *db_owner_name;
int role_oid;
int rolename_len;

user_name = get_physical_user_name(db_name, rolspec->rolename, false);
db_owner_name = get_db_owner_name(get_cur_db_name());
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: simply use db_name instead of get_cur_db_name

role_oid = get_role_oid(user_name, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

why missing_ok = true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For if exist case which is already present in some testfiles

rolename_len = strlen(rolspec->rolename);

if (drop_user)
db_principal = "user";
else
db_principal = "role";

/* If user is dbo or role is db_owner, restrict dropping */
if ((drop_user && rolename_len == 3 && strncmp(rolspec->rolename, "dbo", 3) == 0) ||
(drop_role && rolename_len == 8 && strncmp(rolspec->rolename, "db_owner", 8) == 0))
ereport(ERROR,
(errcode(ERRCODE_CHECK_VIOLATION),
errmsg("Cannot drop the %s '%s'.", db_principal, rolspec->rolename)));

/*
* Check for current_user's privileges
* must be database owner to drop user/role
*/
if ((!stmt->missing_ok && !OidIsValid(role_oid)) ||
!is_member_of_role(GetUserId(), get_role_oid(db_owner_name, false)))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("Cannot drop the %s '%s', because it does not exist or you do not have permission.", db_principal, rolspec->rolename)));

/*
* If a role has members, do not drop it.
* Note that here we don't handle invalid
* roles.
*/
if (drop_role && !is_empty_role(get_role_oid(user_name, true)))
if (drop_role && !is_empty_role(role_oid))
ereport(ERROR,
(errcode(ERRCODE_CHECK_VIOLATION),
errmsg("The role has members. It must be empty before it can be dropped.")));
Expand Down
2 changes: 1 addition & 1 deletion test/JDBC/expected/BABEL-3844-vu-cleanup.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ drop user test_user;
GO
~~ERROR (Code: 33557097)~~

~~ERROR (Message: role "master_test_user" does not exist)~~
~~ERROR (Message: Cannot drop the user 'test_user', because it does not exist or you do not have permission.)~~


drop login [babel\aduser1];
Expand Down
228 changes: 228 additions & 0 deletions test/JDBC/expected/restrict_drop_user_role.out
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need separate test files? Let's reuse some existing test files. IIRC we should avoid creating new test files wherever possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don’t have any such testfile for this usecase hence created one for this usecase testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant wherever we have tests for restricting create login/user, we can use that file.

Copy link
Contributor

Choose a reason for hiding this comment

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

please add some tests which involves if exists, for example, drop user if exists username

Original file line number Diff line number Diff line change
@@ -0,0 +1,228 @@
-- tsql
create login no_priv_login1 with password = '12345678'
go

create role dont_drop_role
go

create login no_priv_login2 with password = '12345678'
go

create user no_priv_user2 for login no_priv_login2
go

create database restrict_user_db1
go

use restrict_user_db1
go

create role dont_drop_role
go

create user no_priv_user1 for login no_priv_login1
go

use master
go

-- tsql user=no_priv_login1 password=12345678
-- case-1 - when current user is guest
-- permission denied
select current_user,db_name()
go
~~START~~
varchar#!#nvarchar
guest#!#master
~~END~~


drop role dont_drop_role
go
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the role 'dont_drop_role', because it does not exist or you do not have permission.)~~


drop user no_priv_user2
go
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the user 'no_priv_user2', because it does not exist or you do not have permission.)~~


-- tsql
select current_user,db_name()
go
~~START~~
varchar#!#nvarchar
dbo#!#master
~~END~~


create user no_priv_user1 for login no_priv_login1
go

-- tsql user=no_priv_login1 password=12345678
-- case-2 - when current user has no privilege
-- permission denied
use master
go

select current_user
go
~~START~~
varchar
no_priv_user1
~~END~~


drop role dont_drop_role
go
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the role 'dont_drop_role', because it does not exist or you do not have permission.)~~


drop user no_priv_user2
go
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the user 'no_priv_user2', because it does not exist or you do not have permission.)~~


-- tsql

-- case-3 - when current login has privilege
-- 3.1 - when login is database owner
Alter authorization on database::restrict_user_db1 to no_priv_login2
go

-- tsql user=no_priv_login2 password=12345678 database=restrict_user_db1
-- allowed
select current_user
go
~~START~~
varchar
dbo
~~END~~


drop role dont_drop_role
go

drop user no_priv_user1
go

-- error
-- try to drop non existing role/user
drop role fake_role
go
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the role 'fake_role', because it does not exist or you do not have permission.)~~


drop user fake_user
go
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the user 'fake_user', because it does not exist or you do not have permission.)~~


drop user db
go
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the user 'db', because it does not exist or you do not have permission.)~~


drop role db_own
go
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the role 'db_own', because it does not exist or you do not have permission.)~~


drop user dbo_u1
go
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the user 'dbo_u1', because it does not exist or you do not have permission.)~~


drop role db_owner_r1
go
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the role 'db_owner_r1', because it does not exist or you do not have permission.)~~


-- should deny
-- try to drop dbo user, db_owner role
drop role db_owner
go
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the role 'db_owner'.)~~


drop user dbo
go
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the user 'dbo'.)~~



create role dont_drop_role
go

create user no_priv_user1 for login no_priv_login1
go

-- tsql
-- 3.2 - when login is sysadmin
-- allowed
drop user no_priv_user1
go

drop user no_priv_user2
go

drop role dont_drop_role
go

-- psql
-- Need to terminate active session before cleaning up the login
SELECT pg_terminate_backend(pid) FROM pg_stat_get_activity(NULL)
WHERE sys.suser_name(usesysid) = 'no_priv_login1' AND backend_type = 'client backend' AND usesysid IS NOT NULL;
go
~~START~~
bool
t
~~END~~



-- tsql
drop login no_priv_login1
go

-- psql
-- Need to terminate active session before cleaning up the login
SELECT pg_terminate_backend(pid) FROM pg_stat_get_activity(NULL)
WHERE sys.suser_name(usesysid) = 'no_priv_login2' AND backend_type = 'client backend' AND usesysid IS NOT NULL;
go
~~START~~
bool
t
~~END~~


-- tsql
drop login no_priv_login2
go

drop database restrict_user_db1
go
Loading
Loading