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

server/auth: implement role & role manager #3256

Merged
merged 10 commits into from Dec 9, 2020

Conversation

PhotonQuantum
Copy link

Signed-off-by: PhotonQuantum self@lightquantum.me

What problem does this PR solve?

ref: tikv/tikv#8621
server/auth: implement role & role manager.

This PR is split from #3224, and moved from #3248.

I'm still working on the RFC document about this change (RBAC support for TiDB PD), and I'll submit it as soon as it's completed. In the meantime, a draft written in Chinese is available at here.

What is changed and how it works?

server/auth roleManager is implemented to handle role-related CRUD operations, including querying roles in memory, loading them from etcd and persisting them to etcd.

Check List

Tests

  • Unit test

Code changes

  • Has persistent data change (etcd)

Release note

  • No release note

errors.toml Outdated
@@ -11,6 +11,31 @@ error = '''
redirect failed
'''

["PD:auth:ErrInvalidName"]
error = '''
key name may only contain alphanumeric and underscores, and may only start with an alphabetic character.
Copy link
Contributor

Choose a reason for hiding this comment

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

An end user won't know what "key" means.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! Fixed.


// Permission represents a permission to a specific pair of resource and action.
type Permission struct {
Resource string `json:"resource"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a newtype like Action as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

We may define Resource as an enum.

@Yisaer
Copy link
Contributor

Yisaer commented Dec 8, 2020

/run-all-tests

Copy link
Contributor

@Yisaer Yisaer left a comment

Choose a reason for hiding this comment

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

Rest LGTM, plz address the comment.

server/auth/util.go Show resolved Hide resolved
server/auth/role.go Outdated Show resolved Hide resolved
server/auth/auth.go Outdated Show resolved Hide resolved
server/auth/auth_test.go Outdated Show resolved Hide resolved
server/auth/auth.go Outdated Show resolved Hide resolved
server/auth/auth.go Outdated Show resolved Hide resolved
type Action string

// All available actions types.
const (
Copy link
Member

Choose a reason for hiding this comment

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

How about using the way like operator kind?

Copy link
Author

Choose a reason for hiding this comment

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

We are assuming that an Action can hold only one operation now (so that AddPermission/RemovePermission can be simple and direct).
Maybe we won't gain much from changing those consts to uints?

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm...I'm not sure if it is reasonable.

server/auth/role.go Outdated Show resolved Hide resolved
Signed-off-by: PhotonQuantum <self@lightquantum.me>
Signed-off-by: PhotonQuantum <self@lightquantum.me>
Signed-off-by: PhotonQuantum <self@lightquantum.me>
server/auth/auth.go Outdated Show resolved Hide resolved
Signed-off-by: PhotonQuantum <self@lightquantum.me>
Signed-off-by: PhotonQuantum <self@lightquantum.me>
Signed-off-by: PhotonQuantum <self@lightquantum.me>
Signed-off-by: PhotonQuantum <self@lightquantum.me>
Signed-off-by: PhotonQuantum <self@lightquantum.me>
@Yisaer
Copy link
Contributor

Yisaer commented Dec 9, 2020

/run-all-tests

Copy link
Contributor

@Yisaer Yisaer left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Yisaer Yisaer left a comment

Choose a reason for hiding this comment

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

LGTM

@zhouqiang-cl
Copy link
Contributor

/run-all-tests

@zhouqiang-cl
Copy link
Contributor

/run-all-tests tidb=master tikv=master

@zhouqiang-cl
Copy link
Contributor

/run-all-tests tidb=master tikv=master tidb-test=master

@zhouqiang-cl
Copy link
Contributor

/run-all-tests tidb=master tikv=master tidb-test=master pd=master

}

// HasPermission checks whether this user has a specific permission.
func (r *Role) HasPermission(permission Permission) bool {
Copy link
Member

Choose a reason for hiding this comment

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

How about making these kinds of methods private?

Copy link
Author

Choose a reason for hiding this comment

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

Good idea! I've made them private.

}
}

func newTestSingleConfig() *embed.Config {
Copy link
Member

Choose a reason for hiding this comment

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

Does NewTestSingleConfig in server package meet the requirement?

Copy link
Author

Choose a reason for hiding this comment

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

Actually, it seems that etcd is not needed to run these tests. I'm replacing EtcdKV with MemoryKV.

Signed-off-by: PhotonQuantum <self@lightquantum.me>
Signed-off-by: PhotonQuantum <self@lightquantum.me>
@Yisaer Yisaer merged commit 8715837 into tikv:rbac-auth Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants