-
Notifications
You must be signed in to change notification settings - Fork 0
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
Pass options separately #2
Conversation
fd04748
to
8c3254a
Compare
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.
Few comments
common.go
Outdated
} | ||
|
||
// newRequest parses a path into an rcpRequest. | ||
func newRequest(path string) *rcpRequest { | ||
func ParseRequest(path string) *Request { |
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.
Lets not export this function if we need it somwhere else, i would copy it, to keep the API of this package simple.
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.
I would rather keep it exported since I'm using it here:
https://github.com/0xsequence/quotacontrol/pull/53/files#diff-7bf08ce0a790451f4677b47d7e17aafcc1825aa92312c83775de6c22e7b5a102R10-R23
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.
This would panic if the path is not valid webrpc route. Because you don't check for nil. And we don't check for nil in Get()
function.
This is not good to export these functions, to be able to just pass the result without checking to another function.
So, i suggest to merge the two functions together. And get rid of the whole webrpcRequest
struct, it is not needed after all.
This is smaller and more understandable code.
// Config is a generic map of services/methods to a config value.
// map[service]map[method]T
type Config[T any] map[string]map[string]T
// Get returns the config value for the given request.
func (c Config[T]) Get(path string) (*T, error) {
if c == nil {
return nil, fmt.Errorf("cofig is nil")
}
p := strings.Split(path, "/")
if len(p) < 4 {
return nil, fmt.Errorf("path has not enough parts")
}
var (
packageName = p[len(p)-3]
serviceName = p[len(p)-2]
methodName = p[len(p)-1]
)
if packageName != "rpc" {
return nil, fmt.Errorf("path doesn't include rpc")
}
methodCfg, ok := c[serviceName][methodName]
if !ok {
return nil, fmt.Errorf("acl not found")
}
return &methodCfg, nil
}
And here: https://github.com/0xsequence/authcontrol/blob/master/middleware.go#L135-L145
Replace it with just:
acl, err := acl.Get(r.URL.Path)
if err != nil {
eh(r, w, proto.ErrUnauthorized.WithCausef("get acl: %w", err))
return
}
@@ -32,45 +36,45 @@ type UserStore interface { | |||
// map[service]map[method]T | |||
type Config[T any] map[string]map[string]T |
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.
Question about this type -- why is it generic? Do we expect to pass in different values to this / can you give me some example pls?
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 generic because it's used for ACL here and for cost in QuotaControl.
See this for instance.
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.
And for what is exactly used the SetCost
function and what represent the int64
value there?
I think we can just type it statically to type Config map[string]map[string]ACL
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.
SetCost sets the call cost for QuotaControl
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.
Second round of CR
func VerifyACL[T any](acl Config[ACL]) error { | ||
var t T | ||
iType := reflect.TypeOf(&t).Elem() | ||
service := iType.Name() | ||
m, ok := acl[service] | ||
if !ok { | ||
return errors.New("service " + service + " not found") | ||
} | ||
var errList []error | ||
for i := 0; i < iType.NumMethod(); i++ { | ||
method := iType.Method(i) | ||
if _, ok := m[method.Name]; !ok { | ||
errList = append(errList, errors.New(""+service+"."+method.Name+" not found")) | ||
} | ||
} | ||
return errors.Join(errList...) | ||
} |
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.
This is IMHO much easier to read, much more understandable and without reflect.
Please use this approach.
Then in tests, you just pass this value: https://github.com/0xsequence/builder/blob/master/api/proto/builder.gen.go#L1794
func VerifyACL[T any](acl Config[ACL]) error { | |
var t T | |
iType := reflect.TypeOf(&t).Elem() | |
service := iType.Name() | |
m, ok := acl[service] | |
if !ok { | |
return errors.New("service " + service + " not found") | |
} | |
var errList []error | |
for i := 0; i < iType.NumMethod(); i++ { | |
method := iType.Method(i) | |
if _, ok := m[method.Name]; !ok { | |
errList = append(errList, errors.New(""+service+"."+method.Name+" not found")) | |
} | |
} | |
return errors.Join(errList...) | |
} | |
// VerifyACL checks that the given ACL config is valid for the given service. | |
// It can be used in unit tests to ensure that all methods are covered. | |
func (acl Config[any]) VerifyACL(webrpcServices map[string][]string) error { | |
var errList []error | |
for service, methods := range webrpcServices { | |
for _, method := range methods { | |
if _, ok := acl[service][method]; !ok { | |
errList = append(errList, fmt.Errorf("%s.%s not found", service, method)) | |
} | |
} | |
} | |
return errors.Join(errList...) | |
} |
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.
This sounds like a good suggestion.
func TestVerifyACL(t *testing.T) { | ||
type Service interface { | ||
Method1() error | ||
Method2() error | ||
Method3() error | ||
} | ||
|
||
err := authcontrol.VerifyACL[Service](nil) | ||
require.Error(t, err) | ||
|
||
err = authcontrol.VerifyACL[Service](authcontrol.Config[authcontrol.ACL]{ | ||
"WrongName": { | ||
"Method1": authcontrol.NewACL(proto.SessionType_User), | ||
"Method2": authcontrol.NewACL(proto.SessionType_User), | ||
"Method3": authcontrol.NewACL(proto.SessionType_User), | ||
}, | ||
}) | ||
require.Error(t, err) | ||
|
||
err = authcontrol.VerifyACL[Service](authcontrol.Config[authcontrol.ACL]{ | ||
"Service": { | ||
"Method1": authcontrol.NewACL(proto.SessionType_User), | ||
"Method2": authcontrol.NewACL(proto.SessionType_User), | ||
}, | ||
}) | ||
require.Error(t, err) | ||
|
||
err = authcontrol.VerifyACL[Service](authcontrol.Config[authcontrol.ACL]{ | ||
"Service": { | ||
"Method1": authcontrol.NewACL(proto.SessionType_User), | ||
"Method2": authcontrol.NewACL(proto.SessionType_User), | ||
"Method3": authcontrol.NewACL(proto.SessionType_User), | ||
}, | ||
}) | ||
require.NoError(t, err) | ||
} |
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.
The tests then can look like this.
func TestVerifyACL(t *testing.T) { | |
type Service interface { | |
Method1() error | |
Method2() error | |
Method3() error | |
} | |
err := authcontrol.VerifyACL[Service](nil) | |
require.Error(t, err) | |
err = authcontrol.VerifyACL[Service](authcontrol.Config[authcontrol.ACL]{ | |
"WrongName": { | |
"Method1": authcontrol.NewACL(proto.SessionType_User), | |
"Method2": authcontrol.NewACL(proto.SessionType_User), | |
"Method3": authcontrol.NewACL(proto.SessionType_User), | |
}, | |
}) | |
require.Error(t, err) | |
err = authcontrol.VerifyACL[Service](authcontrol.Config[authcontrol.ACL]{ | |
"Service": { | |
"Method1": authcontrol.NewACL(proto.SessionType_User), | |
"Method2": authcontrol.NewACL(proto.SessionType_User), | |
}, | |
}) | |
require.Error(t, err) | |
err = authcontrol.VerifyACL[Service](authcontrol.Config[authcontrol.ACL]{ | |
"Service": { | |
"Method1": authcontrol.NewACL(proto.SessionType_User), | |
"Method2": authcontrol.NewACL(proto.SessionType_User), | |
"Method3": authcontrol.NewACL(proto.SessionType_User), | |
}, | |
}) | |
require.NoError(t, err) | |
} | |
func TestVerifyACL(t *testing.T) { | |
services := map[string][]string{ | |
"Service1": { | |
"Method1", | |
"Method2", | |
"Method3", | |
}, | |
"Service2": { | |
"Method1", | |
}, | |
} | |
// Valid ACL config | |
acl := authcontrol.Config[any]{ | |
"Service1": { | |
"Method1": authcontrol.NewACL(proto.SessionType_User), | |
"Method2": authcontrol.NewACL(proto.SessionType_User), | |
"Method3": authcontrol.NewACL(proto.SessionType_User), | |
}, | |
"Service2": { | |
"Method1": authcontrol.NewACL(proto.SessionType_User), | |
}, | |
} | |
err := acl.VerifyACL(services) | |
assert.NoError(t, err) | |
// Wrong Service | |
acl = authcontrol.Config[any]{ | |
"WrongService1": { | |
"Method1": authcontrol.NewACL(proto.SessionType_User), | |
"Method2": authcontrol.NewACL(proto.SessionType_User), | |
"Method3": authcontrol.NewACL(proto.SessionType_User), | |
}, | |
"Service2": { | |
"Method1": authcontrol.NewACL(proto.SessionType_User), | |
}, | |
} | |
err = acl.VerifyACL(services) | |
require.Error(t, err) | |
expectedErrors := []error{ | |
errors.New("Service1.Method1 not found"), | |
errors.New("Service1.Method2 not found"), | |
errors.New("Service1.Method3 not found"), | |
} | |
assert.Equal(t, errors.Join(expectedErrors...).Error(), err.Error()) | |
// Wrong Methods | |
acl = authcontrol.Config[any]{ | |
"Service1": { | |
"Method1": authcontrol.NewACL(proto.SessionType_User), | |
}, | |
"Service2": { | |
"Method1": authcontrol.NewACL(proto.SessionType_User), | |
}, | |
} | |
err = acl.VerifyACL(services) | |
require.Error(t, err) | |
expectedErrors = []error{ | |
errors.New("Service1.Method2 not found"), | |
errors.New("Service1.Method3 not found"), | |
} | |
assert.Equal(t, errors.Join(expectedErrors...).Error(), err.Error()) | |
} |
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.
looks good too
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.
Let's merge this as is, good work. @david-littlefarmer you had some good suggestions too, mind create a separate PR with those changes? 👍
Let's merge ✅
The options as argument leads to issue when using it in other packages.