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

Enhancements for db_securityadmin fixed role #3344

Conversation

HarshLunagariya
Copy link
Contributor

@HarshLunagariya HarshLunagariya commented Jan 2, 2025

Description

Allow UPDATE on sequence and allow add/dropping current user(which is member of db_securityadmin) from user-defined database roles. Added some more tests.

Issues Resolved

BABEL-5135

Test Scenarios Covered

  • Use case based -
    Added

  • 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.

@coveralls
Copy link
Collaborator

coveralls commented Jan 2, 2025

Pull Request Test Coverage Report for Build 12595724260

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 74.865%

Totals Coverage Status
Change from base Build 12582871854: 0.0%
Covered Lines: 46600
Relevant Lines: 62245

💛 - Coveralls

Comment on lines +192 to +196
ALTER ROLE babel_5135_r1 ADD MEMBER babel_5135_dbsecadmin_u1;
GO

ALTER ROLE babel_5135_r1 DROP MEMBER babel_5135_dbsecadmin_u1;
GO
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).

Comment on lines +185 to +189
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)~~
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

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.

Comment on lines +198 to +202
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)~~
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.

Signed-off-by: Harsh Lunagariya <[email protected]>
@rishabhtanwar29 rishabhtanwar29 merged commit 0b77a7a into babelfish-for-postgresql:BABEL_5_X_DEV Jan 3, 2025
45 checks passed
@rishabhtanwar29 rishabhtanwar29 deleted the babel_secadmin branch January 3, 2025 10:48
pranavJ23 pushed a commit to pranavJ23/babelfish_extensions that referenced this pull request Jan 6, 2025
…l#3344)

Allow UPDATE on sequence and allow add/dropping current user(which is member of db_securityadmin) from user-defined database roles. Added some more tests.

Task: BABEL-5135
Signed-off-by: Harsh Lunagariya <[email protected]>
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