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

feat(rbac): drop role will transfer role owns object to account_admin #15154

Merged
merged 7 commits into from
Apr 9, 2024

Conversation

TCeason
Copy link
Collaborator

@TCeason TCeason commented Apr 2, 2024

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

drop role will transfer role owns object to account_admin

E.g.

create role role1;
create user u1 identified by '123' with default_role='role1';
grant role role1 to u1;

use u1 create some database then, drop the role1. In ownership-kv, the object keys value is a dropped role role1.

If admin user wants to create a new role also named role1, the new role role1 will directly has objects ownership. That maybe dangerous.

So in this pr, add a check: dorp role will check the dropped role owns some object. If true, transfer the owner to account_admin.

Note

Add forward/backward compat test.

drop role can execute in old version(1.0.307) and new version.

  • Fixes #[Link the issue here]

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

@TCeason TCeason changed the title feature(rbac): drop role will transfer role owns object to account_admin feat(rbac): drop role will transfer role owns object to account_admin Apr 2, 2024
@github-actions github-actions bot added the pr-feature this PR introduces a new feature to the codebase label Apr 2, 2024
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Consider evaluate the result returned by upsert_kv

@TCeason TCeason requested a review from drmingdrmer April 3, 2024 08:34
src/query/management/src/role/role_api.rs Outdated Show resolved Hide resolved
src/query/management/src/role/role_mgr.rs Outdated Show resolved Hide resolved
src/query/management/src/role/role_mgr.rs Outdated Show resolved Hide resolved
2. add add condition to match seq and key
@TCeason TCeason added the ci-cloud Build docker image for cloud test label Apr 9, 2024
Copy link
Contributor

github-actions bot commented Apr 9, 2024

Docker Image for PR

  • tag: pr-15154-3d6d7eb

note: this image tag is only available for internal use,
please check the internal doc for more details.

@TCeason
Copy link
Collaborator Author

TCeason commented Apr 9, 2024

cc @BohuTANG Already add compat test. will test drop role in old and new version. And I test the drop role in cloud dev.

This is the process:

│ tc-test1 │ Suspended │ XSmall │ │ 60 │ NULL │ 2024-04-09 03:30:02 │
│ tc-test2 │ Suspended │ XSmall │ pr-15154-3d6d7eb │ 60 │ NULL │ 2024-04-09 05:56:17 │

tc-test1 is old warehouse,
tc-test2 is this pr's version warehouse.

➜  ~👆 export BENDSQL_DSN="***&warehouse=tc-test1"
➜  ~👆 bendsql

> create role role1;
Processed in (0.738 sec)
> grant ownership on default.t to role role1;

➜  ~👆 export BENDSQL_DSN="***&warehouse=tc-test2"
➜  ~👆 bendsql

> drop role role1;
Processed in (0.732 sec)

-- the table t transfer to role account_admin
> select name, owner from system.tables where name='t' and database='default';
┌───────────────────────────┐
│  name  │       owner      │
│ String │ Nullable(String) │
├────────┼──────────────────┤
│ t      │ account_admin    │
└───────────────────────────┘

@BohuTANG BohuTANG merged commit d72f6aa into datafuselabs:main Apr 9, 2024
80 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-cloud Build docker image for cloud test pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants