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

anju15bharti · 2024-08-14T16:15:42Z

Description

Earlier, any user was able to drop user/role, irrespective of whether that user has required privileges or not.

With this commit, only dbo should have the permission to drop user/role.

Issues Resolved

BABEL-5173

Test Scenarios Covered

Use case based -
Boundary conditions -
Arbitrary inputs -
Negative test cases -
Minor version upgrade tests -
Major version upgrade tests -
Performance tests -
Tooling impact -
Client tests -

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.

Signed-off-by: ANJU BHARTI <[email protected]>

coveralls · 2024-08-14T17:10:55Z

Pull Request Test Coverage Report for Build 10453221297

Details

15 of 15 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 74.067%

Totals
Change from base Build 10389490935:	0.02%
Covered Lines:	44449
Relevant Lines:	60012

💛 - Coveralls

HarshLunagariya · 2024-08-14T18:29:13Z

contrib/babelfishpg_tsql/src/pl_handler.c

+						if (!has_privs_of_role(GetUserId(),get_role_oid(db_owner_name, false)))
+							ereport(ERROR,
+									(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+									 errmsg("User does not have permission to perform this action.")));


Is this aligning with ideal T-SQL behaviour? Error msg is matching?

HarshLunagariya · 2024-08-14T18:34:08Z

contrib/babelfishpg_tsql/src/pl_handler.c

+
+						/* must be database owner to drop user/role*/
+						db_owner_name = get_db_owner_name(get_cur_db_name());
+						if (!has_privs_of_role(GetUserId(),get_role_oid(db_owner_name, false)))


nit: ...(GetUserId(),<WHITESPACE> get_role_oid...

HarshLunagariya · 2024-08-14T18:38:34Z

contrib/babelfishpg_tsql/src/pl_handler.c

+
+						/* must be database owner to drop user/role*/
+						db_owner_name = get_db_owner_name(get_cur_db_name());
+						if (!has_privs_of_role(GetUserId(),get_role_oid(db_owner_name, false)))


Why has_privs_of_role , not is_member_of_role?

Because this is what we are checking when creating the role/user.

HarshLunagariya

Overall changes looks good.

contrib/babelfishpg_tsql/src/pl_handler.c

Signed-off-by: ANJU BHARTI <[email protected]>

…-5173

Signed-off-by: ANJU BHARTI <[email protected]>

HarshLunagariya · 2024-08-16T07:47:46Z

contrib/babelfishpg_tsql/src/pl_handler.c

+									if ((drop_user && strncmp(logical_role_name, "dbo", rolename_len) == 0) ||
+										(drop_role && strncmp(logical_role_name, "db_owner", rolename_len) == 0))


What if this check is run against user name d or db_o or db_ow etc.? We need to have the length comparison before using strncmp().

Please add such testcases.

HarshLunagariya · 2024-08-16T07:51:27Z

contrib/babelfishpg_tsql/src/pl_handler.c

+									const char *db_owner_name;
+									int	      	role_oid;
+									int       	rolename_len;
+									char	   *logical_role_name = NULL;


logical_role_name is not needed IMO. Let's use rolespec->rolename only.

Signed-off-by: ANJU BHARTI <[email protected]>

shalinilohia50 · 2024-08-17T08:04:03Z

contrib/babelfishpg_tsql/src/pl_handler.c

@@ -3285,15 +3285,44 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
 								{
 									RoleSpec   *rolspec = lfirst(item);
 									char	   *user_name;
+									char       *db_principal;


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

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

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

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

shalinilohia50 · 2024-08-17T08:06:06Z

test/JDBC/expected/restrict_drop_user_role.out

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

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

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

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

shalinilohia50

LGTM!

Deepesh125 · 2024-08-19T05:42:44Z

contrib/babelfishpg_tsql/src/pl_handler.c


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


Nit: simply use db_name instead of get_cur_db_name

Deepesh125 · 2024-08-19T05:46:23Z

contrib/babelfishpg_tsql/src/pl_handler.c


 									user_name = get_physical_user_name(db_name, rolspec->rolename, false);
+									db_owner_name = get_db_owner_name(get_cur_db_name());
+									role_oid = get_role_oid(user_name, true);


why missing_ok = true?

For if exist case which is already present in some testfiles

Deepesh125 · 2024-08-19T05:49:12Z

contrib/babelfishpg_tsql/src/pl_handler.c

@@ -3285,15 +3285,44 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
 								{
 									RoleSpec   *rolspec = lfirst(item);
 									char	   *user_name;
+									char       *db_principal;


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

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

Deepesh125 · 2024-08-19T05:55:01Z

test/JDBC/expected/restrict_drop_user_role.out

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

Signed-off-by: Harsh Lunagariya <[email protected]>

Deepesh125 · 2024-08-19T10:41:54Z

contrib/babelfishpg_tsql/src/pl_handler.c


 									user_name = get_physical_user_name(db_name, rolspec->rolename, false);
+									db_owner_name = get_db_owner_name(db_name);


we should check whether user_name is valid T-SQL role or login.

Signed-off-by: Harsh Lunagariya <[email protected]>

HarshLunagariya · 2024-08-19T13:35:47Z

Description

Earlier, any user was able to drop user/role, irrespective of whether that user has required privileges or not.

With this commit, only dbo should have the permission to drop user/role.

Issues Resolved

BABEL-5173

Description
Earlier, any user was able to drop user/role, irrespective of whether that user has required privileges or not.

With this commit, Only dbo and members of db_owner will have the permission to drop user/role. 
Additionally, this restricts dropping internal database principal such as dbo and db_owner, it restricts
dropping non-Babelfish roles from TDS endpoint.

Issues Resolved
BABEL-5173

) Earlier, any user was able to drop user/role, irrespective of whether that user has required privileges or not. With this commit, Only dbo and members of db_owner will have the permission to drop user/role. Additionally, this restricts dropping internal database principal such as dbo and db_owner, it restricts dropping non-Babelfish roles from TDS endpoint. Task: BABEL-5173 Authored-by: ANJU BHARTI <[email protected]> Co-authored-by: Harsh Lunagariya <[email protected]> Signed-off-by: Harsh Lunagariya <[email protected]>

Earlier, a user was able to drop user/role that belonged to another database. With this commit, a user can only drop the role/user that belongs to the same database with sufficient privileges. Issues Resolved Task: BABEL-5173 Signed-off-by: Shalini Lohia [email protected]

) Earlier, any user was able to drop user/role, irrespective of whether that user has required privileges or not. With this commit, Only dbo and members of db_owner will have the permission to drop user/role. Additionally, this restricts dropping internal database principal such as dbo and db_owner, it restricts dropping non-Babelfish roles from TDS endpoint. Task: BABEL-5173 Authored-by: ANJU BHARTI <[email protected]> Co-authored-by: Harsh Lunagariya <[email protected]> Signed-off-by: Harsh Lunagariya <[email protected]>

) (babelfish-for-postgresql#2864) Earlier, a user was able to drop user/role that belonged to another database. With this commit, a user can only drop the role/user that belongs to the same database with sufficient privileges. Issues Resolved Task: BABEL-5173 Signed-off-by: Shalini Lohia [email protected]

) (babelfish-for-postgresql#2865) Earlier, a user was able to drop user/role that belonged to another database. With this commit, a user can only drop the role/user that belongs to the same database with sufficient privileges. Issues Resolved Task: BABEL-5173 Signed-off-by: Shalini Lohia [email protected]

Earlier, any user was able to drop user/role, irrespective of whether that user has required privileges or not. With this commit, Only dbo and members of db_owner will have the permission to drop user/role. Additionally, this restricts dropping internal database principal such as dbo and db_owner, it restricts dropping non-Babelfish roles from TDS endpoint. Task: BABEL-5173 Authored-by: ANJU BHARTI <[email protected]>

Earlier, a user was able to drop user/role that belonged to another database. With this commit, a user can only drop the role/user that belongs to the same database with sufficient privileges. Issues Resolved Task: BABEL-5173 Signed-off-by: Shalini Lohia [email protected]

) (babelfish-for-postgresql#2864) Earlier, a user was able to drop user/role that belonged to another database. With this commit, a user can only drop the role/user that belongs to the same database with sufficient privileges. Issues Resolved Task: BABEL-5173 Signed-off-by: Shalini Lohia [email protected]

Restrict DROP USER/ROLE from non-dbo user

b946273

Signed-off-by: ANJU BHARTI <[email protected]>

HarshLunagariya reviewed Aug 14, 2024

View reviewed changes

Deepesh125 requested changes Aug 15, 2024

View reviewed changes

contrib/babelfishpg_tsql/src/pl_handler.c Outdated Show resolved Hide resolved

Deepesh125 previously approved these changes Aug 15, 2024

View reviewed changes

anju15bharti dismissed Deepesh125’s stale review via ba94f77 August 15, 2024 08:52

anju15bharti force-pushed the jira-BABEL-5173 branch from ba94f77 to 1734382 Compare August 15, 2024 09:29

Restrict dropping of dbo and db_owner

ec5d2b5

Signed-off-by: ANJU BHARTI <[email protected]>

anju15bharti force-pushed the jira-BABEL-5173 branch from 1734382 to ec5d2b5 Compare August 15, 2024 09:30

ANJU BHARTI added 2 commits August 16, 2024 03:24

Updated BABEL-3844-vu-cleanup expected file

149ad22

Signed-off-by: ANJU BHARTI <[email protected]>

Merge remote-tracking branch 'upstream/BABEL_4_X_DEV' into jira-BABEL…

799df76

…-5173

anju15bharti force-pushed the jira-BABEL-5173 branch from 63ee771 to 7c1b926 Compare August 16, 2024 05:58

Replaced physical rolename with logical rolename in error message

675f72e

Signed-off-by: ANJU BHARTI <[email protected]>

anju15bharti force-pushed the jira-BABEL-5173 branch from 7c1b926 to 675f72e Compare August 16, 2024 06:07

HarshLunagariya reviewed Aug 16, 2024

View reviewed changes

Added more testcases

4330cdd

Signed-off-by: ANJU BHARTI <[email protected]>

HarshLunagariya approved these changes Aug 16, 2024

View reviewed changes

shalinilohia50 reviewed Aug 17, 2024

View reviewed changes

shalinilohia50 approved these changes Aug 19, 2024

View reviewed changes

Deepesh125 requested changes Aug 19, 2024

View reviewed changes

Address comments

64538e6

Signed-off-by: Harsh Lunagariya <[email protected]>

Deepesh125 requested changes Aug 19, 2024

View reviewed changes

Address more comments

918f087

Signed-off-by: Harsh Lunagariya <[email protected]>

shalinilohia50 requested a review from Deepesh125 August 19, 2024 13:22

Deepesh125 approved these changes Aug 19, 2024

View reviewed changes

Deepesh125 merged commit a66de39 into babelfish-for-postgresql:BABEL_4_X_DEV Aug 19, 2024
44 checks passed

Deepesh125 deleted the jira-BABEL-5173 branch August 19, 2024 13:46

This was referenced Aug 26, 2024

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

Merged

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

anju15bharti commented Aug 14, 2024

coveralls commented Aug 14, 2024 •

edited

Loading

HarshLunagariya Aug 14, 2024 •

edited

Loading

HarshLunagariya Aug 14, 2024

HarshLunagariya Aug 14, 2024

anju15bharti Aug 15, 2024

HarshLunagariya left a comment

HarshLunagariya Aug 16, 2024

HarshLunagariya Aug 16, 2024

HarshLunagariya Aug 16, 2024

shalinilohia50 Aug 17, 2024

anju15bharti Aug 17, 2024

Deepesh125 Aug 19, 2024

shalinilohia50 Aug 17, 2024

anju15bharti Aug 17, 2024

shalinilohia50 Aug 19, 2024

Deepesh125 Aug 19, 2024

shalinilohia50 left a comment

Deepesh125 Aug 19, 2024

Deepesh125 Aug 19, 2024

anju15bharti Aug 19, 2024

Deepesh125 Aug 19, 2024

Deepesh125 Aug 19, 2024

Deepesh125 Aug 19, 2024

HarshLunagariya commented Aug 19, 2024

Description

Issues Resolved

		if ((drop_user && strncmp(logical_role_name, "dbo", rolename_len) == 0) \|\|
		(drop_role && strncmp(logical_role_name, "db_owner", rolename_len) == 0))


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


		user_name = get_physical_user_name(db_name, rolspec->rolename, false);
		db_owner_name = get_db_owner_name(db_name);

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

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

Conversation

anju15bharti commented Aug 14, 2024

Description

Issues Resolved

Test Scenarios Covered

Check List

coveralls commented Aug 14, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10453221297

Details

💛 - Coveralls

HarshLunagariya Aug 14, 2024 • 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

HarshLunagariya left a 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

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 left a 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

HarshLunagariya commented Aug 19, 2024

Description

Issues Resolved

coveralls commented Aug 14, 2024 •

edited

Loading

HarshLunagariya Aug 14, 2024 •

edited

Loading