Skip to content

privilege, executor: add SET ROLE and CURRENT_ROLE support#9581

Merged
imtbkcat merged 15 commits intopingcap:masterfrom
imtbkcat:setrole
Mar 21, 2019
Merged

privilege, executor: add SET ROLE and CURRENT_ROLE support#9581
imtbkcat merged 15 commits intopingcap:masterfrom
imtbkcat:setrole

Conversation

@imtbkcat
Copy link
Copy Markdown

@imtbkcat imtbkcat commented Mar 7, 2019

What problem does this PR solve?

support active role and SET ROLE, CURRENT_ROLE function.

What is changed and how it works?

Create an graph data structure to find relationship between role and user quickly, which is always update as other privilege tables. It will load mysql.role_edges table, and convert relationship to an graph.

When we need to active some roles for current session, we need to check whether these roles has been granted for current user. RoleGraph can finish this task quickly.

SET ROLE is just for set active role for current session, more detail: https://dev.mysql.com/doc/refman/8.0/en/set-role.html

Because set default role is not support yet. some gramma will be added soon.

Check List

Tests

  • Unit test

Code changes

  • Has exported variable/fields change

Side effects

  • Increased code complexity

Comment thread privilege/privileges/cache.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Move this line to the third party libs part.

Comment thread expression/builtin_info.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is better to add some test cases for this built-in function.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ok

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 9, 2019

Codecov Report

Merging #9581 into master will decrease coverage by 0.0201%.
The diff coverage is 44.8979%.

@@               Coverage Diff                @@
##             master      #9581        +/-   ##
================================================
- Coverage   67.1706%   67.1505%   -0.0202%     
================================================
  Files           381        381                
  Lines         79956      80053        +97     
================================================
+ Hits          53707      53756        +49     
- Misses        21460      21504        +44     
- Partials       4789       4793         +4

Comment thread executor/simple.go Outdated
Comment thread privilege/privileges/cache.go Outdated
Comment thread privilege/privileges/cache.go Outdated
Comment thread privilege/privileges/cache.go Outdated
Comment thread privilege/privileges/cache.go Outdated
@imtbkcat
Copy link
Copy Markdown
Author

/run-all-tests

@imtbkcat
Copy link
Copy Markdown
Author

PTAL @tiancaiamao

Comment thread executor/simple.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

make(map[string]*auth.RoleIdentity, len(s.RoleList))

Comment thread go.mod Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why protobuf is changed here?

Comment thread privilege/privileges/cache.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's better to use roleList map[string]struct{} here

roleList map[string]struct{}

roleList["xx"] = struct{}{}

if _, ok := roleList["xx"] {
    ...
}

@tiancaiamao
Copy link
Copy Markdown
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 18, 2019
@imtbkcat
Copy link
Copy Markdown
Author

PTAL @jackysp

@imtbkcat
Copy link
Copy Markdown
Author

/run-all-tests

Copy link
Copy Markdown
Contributor

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@imtbkcat imtbkcat merged commit 778c3f4 into pingcap:master Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/privilege sig/execution SIG execution status/LGT1 Indicates that a PR has LGTM 1.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants