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

RFC: etcdserver: check permission in Txn() RPC #5666

Closed
wants to merge 1 commit into from

Conversation

mitake
Copy link
Contributor

@mitake mitake commented Jun 14, 2016

This PR adds permission checking in Txn() RPC. Currently, the check works like below:

  1. check compare requests and return permission denied error if the requests has denied range operations
  2. check one of the request sequences for success and failure cases and return permission denied error if the requests has denied operations

It means that the transaction request can be completed even if the request sequences that aren't executed has invalid permission. I think it is ok but how do you think? @xiang90 @heyitsanthony

@@ -110,7 +110,7 @@ func (s *EtcdServer) applyV3Request(r *pb.InternalRaftRequest) *applyResult {
ar.err = auth.ErrPermissionDenied
}
case r.Txn != nil:
ar.resp, ar.err = s.applyV3.Txn(r.Txn)
ar.resp, ar.err = s.applyV3.Txn(r.Header, r.Txn)
Copy link
Contributor

Choose a reason for hiding this comment

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

s.applyV3.Txn(ctx context.Context, username string, r.Txn)

@mitake
Copy link
Contributor Author

mitake commented Jun 15, 2016

@heyitsanthony the idea of authApplierV3 seems to be elegant but adding a new argument of auth would make it complex (maybe I'm missing something?). Like @xiang90 said, adding a single function for permission checking and call it like other existing permission checking functions (e.g. IsPutPermitted()) would be the simplest design. How do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants