Pass options separately

[klaidliadon]

The options as argument leads to issue when using it in other packages.

[david-littlefarmer]

Few comments

[david-littlefarmer]

Lets not export this function if we need it somwhere else, i would copy it, to keep the API of this package simple.

[klaidliadon]

I would rather keep it exported since I'm using it here:
https://github.com/0xsequence/quotacontrol/pull/53/files#diff-7bf08ce0a790451f4677b47d7e17aafcc1825aa92312c83775de6c22e7b5a102R10-R23

[david-littlefarmer]

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
}

[VojtechVitek]

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?

[klaidliadon]

It's generic because it's used for ACL here and for cost in QuotaControl.
See this for instance.

[david-littlefarmer]

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

[klaidliadon]

SetCost sets the call cost for QuotaControl

[david-littlefarmer]

Second round of CR

[david-littlefarmer]

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

Suggested change

	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...)
	}

[VojtechVitek]

This sounds like a good suggestion.

[david-littlefarmer]

The tests then can look like this.

Suggested change

	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())
	}

[VojtechVitek]

looks good too

[VojtechVitek]

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 ✅