-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Restrict KV endpoint to root and node users. #2193
Conversation
func (s *Server) Register(name string, handler func(proto.Message) (proto.Message, error), | ||
// If 'restricted' is true, only system users (RootUser and NodeUser) | ||
// may call this method. | ||
func (s *Server) Register(name string, restricted bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than a restricted
argument here, how about leaving Register
as-is and adding a RegisterPublic
method for use by sql
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
The structure looks fine to me, though I might use the term |
@@ -388,7 +396,10 @@ func (s *Server) readRequest(codec rpc.ServerCodec) (req rpc.Request, m method, | |||
if ok { | |||
args = reflect.New(m.reqType.Elem()).Interface().(proto.Message) | |||
} | |||
err = codec.ReadRequestBody(args) | |||
if err = codec.ReadRequestBody(args); err != nil || args == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: i'd put the args != nil
check before the call to authHook rather than here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
LGTM |
adjusted for comments. Will ping again when this is ready. |
Work towards #2089 * restrict all RPC endpoints to root and node users only. * restrict http kv endpoint to root and node users only. * sql http endpoint allows all users Nothing makes use of the rpc.RegisterPublic method yet, but sql will.
25961c9
to
83dfb1b
Compare
This is ready. Except for addressing the review comments, all other changes are to tests. |
LGTM, though I didn't examine the test changes/coverage in depth. |
if err != nil { | ||
t.Fatalf("%s %s: error building request: %s", method, url, err) | ||
} | ||
if b != nil { | ||
req.Header.Add(util.ContentTypeHeader, util.ProtoContentType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: any reason not to always do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's accepted form that the content-type should be set when there is a body. When there is not, it doesn't actually hurt to set it, but there's really no reason to.
LGTM |
Restrict KV endpoint to root and node users.
Work towards #2089
Nothing makes use of the rpc.RegisterPublic method yet, but sql will.