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.
Rather than a restricted argument here, how about leaving Register as-is and adding a RegisterPublic method for use by sql.
|
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.
nit: i'd put the args != nil check before the call to authHook rather than here
|
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.
mberhault
force-pushed
the
marc/restrict_kv_endpoint
branch
from
25961c9
to
83dfb1b
Compare
mberhault
changed the title
|
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.
nit: any reason not to always do this?
There was a problem hiding this comment.
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.