Skip to content

[DPE-7520] Test that the charmed_read role cannot write data #945

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

Jump to bottom
Merged
merged 74 commits into from
Jun 13, 2025
Merged

[DPE-7520] Test that the charmed_read role cannot write data #945

merged 74 commits into from
Jun 13, 2025
+14 −0

Conversation

marceloneppel
Copy link
Member

@marceloneppel marceloneppel commented Jun 9, 2025 •
edited
Loading

Issue

Currently, we only test that the charmed_read role can read data, and not that it can’t write data.

Solution

Add a check in the current integration test to verify that the role cannot write data.

Checklist

  • I have added or updated any relevant documentation.
  • I have cleaned any remaining cloud resources from my accounts.

shayancanonical and others added 30 commits May 6, 2025 10:58
@shayancanonical
Implement instance level predefined roles
e1a14e0
@shayancanonical
Merge branch '16/edge' into feature/16_predefined_instance_roles
23c1ea0
@shayancanonical
Fix minor bug introduced while rebasing off of 16/edge
a5c6442
@shayancanonical
Add integration test for charmed_read and charmed_dml roles
09c4a71
@shayancanonical
Revert all major changes except introduction of predefined roles
d778bba
@shayancanonical
Sweep diff and minor bug fixes
873132e
@shayancanonical
Avoid creating set_user extension
0dc8d3d
@shayancanonical
Merge branch '16/edge' into feature/16_predefined_instance_roles
1764971
@shayancanonical
Port Carl's fix for broken unit tests
fead5cd
@shayancanonical
Merge branch '16/edge' into feature/16_predefined_instance_roles
2c0f822
@shayancanonical
Merge branch '16/edge' into feature/16_predefined_instance_roles
c848782
@marceloneppel
Create set_up_predefined_catalog_roles_function
e49e999 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Fix linting and run function on database creation
5724320 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Add login hook function
8da9ab5 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Escalate relation users
e4bc8d2 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@shayancanonical
Merge branch '16/edge' into feature/16_predefined_instance_roles
8094e4f
@marceloneppel
Add integration test
2f204a9 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Fix unit test
0dd22ed 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Merge remote-tracking branch 'origin/feature/16_predefined_instance_r…
d453455 
…oles' into feature/16_predefined_catalog_roles

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Check for no write permissions for relation user
2fc6e5c 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Don't set up catalog roles if they already exist
fea0148 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Test database creation permission
1c5a911 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Improve logs and move cleanup process to the beginning of the test
6a083c5 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Wait for relation to be removed and retrieve primary
16d6783 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Handle re-relation
fe5ce8f 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Add test for removing and re-adding relation
43493d4 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Test roles after database re-creation
1005f7e 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Test table creation failure for charmed_databases_owner user
c526616 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Deduplicate relations retrieval code
5de77d0 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Check that the relation user can escalate to the database owner user …
ff5dd37 
…and create a table

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Merge remote-tracking branch 'origin/feature/16_predefined_roles_clea…
4084579 
…nup' into feature/16_predefined_catalog_roles

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Merge remote-tracking branch 'origin/16/edge' into feature/16_predefi…
f3eceba 
…ned_dba_role

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Merge remote-tracking branch 'origin/feature/16_predefined_dba_role' …
9721e91 
…into feature/16_predefined_roles_cleanup

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Merge remote-tracking branch 'origin/feature/16_predefined_roles_clea…
7f98938 
…nup' into feature/16_predefined_catalog_roles

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Merge remote-tracking branch 'origin/16/edge' into feature/16_predefi…
40c2051 
…ned_dba_role

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Merge remote-tracking branch 'origin/feature/16_predefined_dba_role' …
d121c1f 
…into feature/16_predefined_roles_cleanup

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Merge remote-tracking branch 'origin/feature/16_predefined_roles_clea…
71fb74f 
…nup' into feature/16_predefined_catalog_roles

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Reset connection to None before creating a new connection
dc5b01e 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Merge remote-tracking branch 'origin/feature/16_predefined_dba_role' …
262dc2b 
…into feature/16_predefined_roles_cleanup

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Remove irrelevant test and increase timeout
0848dcf 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Merge remote-tracking branch 'origin/feature/16_predefined_roles_clea…
32f4f56 
…nup' into feature/16_predefined_catalog_roles

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Merge remote-tracking branch 'origin/16/edge' into feature/16_predefi…
24605d9 
…ned_roles_cleanup

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Merge remote-tracking branch 'origin/feature/16_predefined_roles_clea…
4f2f0e2 
…nup' into feature/16_predefined_catalog_roles

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Merge remote-tracking branch 'origin/16/edge' into feature/16_predefi…
32663cb 
…ned_catalog_roles

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Test that the charmed_read role cannot write data
71feb7b 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel marceloneppel added the not bug or enhancement PR is not 'bug' or 'enhancement'. For release notes label Jun 9, 2025
@codecov Codecov
Copy link

codecov bot commented Jun 9, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.13%. Comparing base (0523e6c) to head (d67738f).
Report is 2 commits behind head on 16/edge.

Additional details and impacted files 
@@           Coverage Diff            @@
##           16/edge     #945   +/-   ##
========================================
  Coverage    70.13%   70.13%           
========================================
  Files           16       16           
  Lines         3817     3817           
  Branches       554      554           
========================================
  Hits          2677     2677           
  Misses        1002     1002           
  Partials       138      138

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Base automatically changed from feature/16_predefined_catalog_roles to 16/edge June 9, 2025 13:33
@marceloneppel
Merge remote-tracking branch 'origin/16/edge' into test-charmed-read-…
c4b1386 
…role-write-data

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel marceloneppel changed the title Test that the charmed_read role cannot write data [DPE-7520] Test that the charmed_read role cannot write data Jun 9, 2025
@marceloneppel marceloneppel marked this pull request as ready for review June 9, 2025 18:31
taurus-forever
taurus-forever approved these changes Jun 10, 2025
View reviewed changes
Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

LGTM, can we expand the testcase here a bit (to sleep better)?

tests/integration/test_predefined_roles.py Outdated
Comment on lines 121 to 124
logger.info("Checking that the charmed_read role cannot write to the database")
with pytest.raises(psycopg2.errors.InsufficientPrivilege):
cursor.execute("CREATE TABLE test_table_2 (id INTEGER);")
connection.close()
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM here, should we also try here to INSERT into test_table?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I'll add that scenario.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done through 99f498c and 5be6d94.

@marceloneppel
Add check for charmed_read role not being able to write data to an ex…
99f498c 
…isting table

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Fix data used to perform insert
5be6d94 
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Merge remote-tracking branch 'origin/16/edge' into test-charmed-read-…
d67738f 
…role-write-data

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel marceloneppel merged commit 6cf8599 into 16/edge Jun 13, 2025
218 of 228 checks passed
@marceloneppel marceloneppel deleted the test-charmed-read-role-write-data branch June 13, 2025 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taurus-forever taurus-forever taurus-forever approved these changes

@dragomirp dragomirp dragomirp approved these changes

Assignees
No one assigned
Labels
not bug or enhancement PR is not 'bug' or 'enhancement'. For release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marceloneppel @taurus-forever @dragomirp @shayancanonical