diff --git a/.mockery.yaml b/.mockery.yaml index 6c2ebb7..20d2471 100644 --- a/.mockery.yaml +++ b/.mockery.yaml @@ -8,3 +8,10 @@ packages: pkgname: authkitmocks dir: mocks/authkit filename: principal_finder.go + PrincipalActionResolver: + config: + template: testify + structname: PrincipalActionResolver + pkgname: authkitmocks + dir: mocks/authkit + filename: principal_action_resolver.go diff --git a/authz/casbin/authorizer.go b/authz/casbin/authorizer.go index f1dbcb7..1610035 100644 --- a/authz/casbin/authorizer.go +++ b/authz/casbin/authorizer.go @@ -7,7 +7,8 @@ import ( "github.com/meigma/authkit" ) -const deniedReason = "casbin policy denied" +// deniedReason is the Decision.Reason returned when the enforcer rejects the request. +const deniedReason = "policy denied" // Enforcer is the Casbin enforcement surface required by Authorizer. type Enforcer interface { @@ -18,6 +19,9 @@ type Enforcer interface { // RequestBuilder projects authkit authorization inputs into Casbin request values. type RequestBuilder func(authkit.AuthorizationCheck) ([]any, error) +// Compile-time assertion that *Authorizer satisfies the authkit.Authorizer port. +var _ authkit.Authorizer = (*Authorizer)(nil) + // Authorizer adapts a Casbin enforcer to authkit.Authorizer. type Authorizer struct { enforcer Enforcer @@ -46,7 +50,8 @@ func NewAuthorizer(enforcer Enforcer, opts ...Option) (*Authorizer, error) { }, nil } -// Can returns the Casbin decision for check. +// Can projects check via the configured RequestBuilder and returns the enforcer's +// authorization decision. func (a *Authorizer) Can(ctx context.Context, check authkit.AuthorizationCheck) (authkit.Decision, error) { if ctxErr := ctx.Err(); ctxErr != nil { return authkit.Decision{}, ctxErr @@ -56,6 +61,8 @@ func (a *Authorizer) Can(ctx context.Context, check authkit.AuthorizationCheck) if err != nil { return authkit.Decision{}, err } + // Re-check after projection so a context cancelled mid-build surfaces as a cancellation + // error rather than running the enforcer against possibly-incomplete request values. if ctxErr := ctx.Err(); ctxErr != nil { return authkit.Decision{}, ctxErr } @@ -75,6 +82,8 @@ func (a *Authorizer) Can(ctx context.Context, check authkit.AuthorizationCheck) } // DefaultRequestBuilder projects check to the classic Casbin sub, obj, act request. +// Missing principal ID, action, or resource type are fail-closed: the projection errors out +// rather than producing an ambiguous Casbin request that could match a permissive policy. func DefaultRequestBuilder(check authkit.AuthorizationCheck) ([]any, error) { if check.Principal.ID == "" { return nil, errors.New("casbin: principal ID is required") @@ -89,7 +98,11 @@ func DefaultRequestBuilder(check authkit.AuthorizationCheck) ([]any, error) { return []any{check.Principal.ID, resourceObject(check.Resource), check.Action}, nil } +// resourceObject renders an authkit.Resource as the Casbin obj value, formatted as +// "type:id" when an ID is present and "type" alone when it is not. func resourceObject(resource authkit.Resource) string { + // Resources without an ID identify a type-level target (e.g. a collection or singleton); + // the policy is expected to match on the type alone in that case. if resource.ID == "" { return resource.Type } diff --git a/authz/casbin/authorizer_test.go b/authz/casbin/authorizer_test.go index 43b7b6d..95624fc 100644 --- a/authz/casbin/authorizer_test.go +++ b/authz/casbin/authorizer_test.go @@ -5,8 +5,6 @@ import ( "errors" "testing" - casbinv3 "github.com/casbin/casbin/v3" - casbinmodel "github.com/casbin/casbin/v3/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -47,79 +45,6 @@ func TestNewAuthorizerValidatesDependencies(t *testing.T) { } } -func TestDefaultRequestBuilderProjectsClassicCasbinRequest(t *testing.T) { - tests := []struct { - name string - resource authkit.Resource - want []any - }{ - { - name: "type and ID", - resource: testResource("note", "note-1"), - want: []any{"principal_1", "note:note-1", "note:update"}, - }, - { - name: "type only", - resource: testResource("system", ""), - want: []any{"principal_1", "system", "note:update"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := DefaultRequestBuilder(testCheck("note:update", tt.resource)) - - require.NoError(t, err) - assert.Equal(t, tt.want, got) - }) - } -} - -func TestDefaultRequestBuilderValidatesRequiredInputs(t *testing.T) { - tests := []struct { - name string - principal authkit.Principal - action string - resource authkit.Resource - want string - }{ - { - name: "missing principal ID", - principal: authkit.Principal{}, - action: "note:update", - resource: testResource("note", "note-1"), - want: "principal ID is required", - }, - { - name: "missing action", - principal: testPrincipal(), - action: "", - resource: testResource("note", "note-1"), - want: "action is required", - }, - { - name: "missing resource type", - principal: testPrincipal(), - action: "note:update", - resource: authkit.Resource{}, - want: "resource type is required", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := DefaultRequestBuilder(authkit.AuthorizationCheck{ - Principal: tt.principal, - Action: tt.action, - Resource: tt.resource, - }) - - require.ErrorContains(t, err, tt.want) - assert.Nil(t, got) - }) - } -} - func TestAuthorizerCanAllowsPolicy(t *testing.T) { var gotRequest []any authorizer := newAuthorizer(t, testEnforcer{ @@ -351,83 +276,3 @@ func TestAuthorizerCanUsesRealCasbinEnforcer(t *testing.T) { assert.Equal(t, authkit.Decision{Allowed: true}, allowed) assert.Equal(t, authkit.Decision{Allowed: false, Reason: deniedReason}, denied) } - -func newAuthorizer(t *testing.T, enforcer Enforcer) *Authorizer { - t.Helper() - - authorizer, err := NewAuthorizer(enforcer) - require.NoError(t, err) - - return authorizer -} - -func newCasbinEnforcer(t *testing.T) *casbinv3.Enforcer { - t.Helper() - - model, err := casbinmodel.NewModelFromString(` -[request_definition] -r = sub, obj, act - -[policy_definition] -p = sub, obj, act - -[policy_effect] -e = some(where (p.eft == allow)) - -[matchers] -m = r.sub == p.sub && r.obj == p.obj && r.act == p.act -`) - require.NoError(t, err) - - enforcer, err := casbinv3.NewEnforcer(model) - require.NoError(t, err) - - return enforcer -} - -func testPrincipal() authkit.Principal { - return authkit.Principal{ - ID: "principal_1", - Kind: authkit.PrincipalKindUser, - DisplayName: "Ada Lovelace", - } -} - -func testCheck(action string, resource authkit.Resource) authkit.AuthorizationCheck { - return authkit.AuthorizationCheck{ - Principal: testPrincipal(), - Action: action, - Resource: resource, - } -} - -func testResource(resourceType string, id string) authkit.Resource { - return authkit.Resource{ - Type: resourceType, - ID: id, - } -} - -type testEnforcer struct { - enforce func(...any) (bool, error) -} - -func (e testEnforcer) Enforce(rvals ...any) (bool, error) { - return e.enforce(rvals...) -} - -func allowEnforcer() testEnforcer { - return testEnforcer{ - enforce: func(...any) (bool, error) { - return true, nil - }, - } -} - -func denyEnforcer() testEnforcer { - return testEnforcer{ - enforce: func(...any) (bool, error) { - return false, nil - }, - } -} diff --git a/authz/casbin/helpers_test.go b/authz/casbin/helpers_test.go new file mode 100644 index 0000000..4119bc1 --- /dev/null +++ b/authz/casbin/helpers_test.go @@ -0,0 +1,91 @@ +package casbin + +import ( + "testing" + + casbinv3 "github.com/casbin/casbin/v3" + casbinmodel "github.com/casbin/casbin/v3/model" + "github.com/stretchr/testify/require" + + "github.com/meigma/authkit" +) + +func newAuthorizer(t *testing.T, enforcer Enforcer) *Authorizer { + t.Helper() + + authorizer, err := NewAuthorizer(enforcer) + require.NoError(t, err) + + return authorizer +} + +func newCasbinEnforcer(t *testing.T) *casbinv3.Enforcer { + t.Helper() + + model, err := casbinmodel.NewModelFromString(` +[request_definition] +r = sub, obj, act + +[policy_definition] +p = sub, obj, act + +[policy_effect] +e = some(where (p.eft == allow)) + +[matchers] +m = r.sub == p.sub && r.obj == p.obj && r.act == p.act +`) + require.NoError(t, err) + + enforcer, err := casbinv3.NewEnforcer(model) + require.NoError(t, err) + + return enforcer +} + +func testPrincipal() authkit.Principal { + return authkit.Principal{ + ID: "principal_1", + Kind: authkit.PrincipalKindUser, + DisplayName: "Ada Lovelace", + } +} + +func testCheck(action string, resource authkit.Resource) authkit.AuthorizationCheck { + return authkit.AuthorizationCheck{ + Principal: testPrincipal(), + Action: action, + Resource: resource, + } +} + +func testResource(resourceType string, id string) authkit.Resource { + return authkit.Resource{ + Type: resourceType, + ID: id, + } +} + +type testEnforcer struct { + enforce func(...any) (bool, error) +} + +func (e testEnforcer) Enforce(rvals ...any) (bool, error) { + return e.enforce(rvals...) +} + +func allowEnforcer() testEnforcer { + return testEnforcer{ + enforce: func(...any) (bool, error) { + return true, nil + }, + } +} + +func denyEnforcer() testEnforcer { + return testEnforcer{ + enforce: func(...any) (bool, error) { + return false, nil + }, + } +} diff --git a/authz/casbin/options.go b/authz/casbin/options.go index d55df7b..414ee4e 100644 --- a/authz/casbin/options.go +++ b/authz/casbin/options.go @@ -1,12 +1,14 @@ package casbin -// Option configures an Authorizer. +// Option configures an Authorizer at construction time. type Option func(*options) +// options holds the resolved configuration applied by NewAuthorizer. type options struct { requestBuilder RequestBuilder } +// defaultOptions returns the baseline configuration used before any Option is applied. func defaultOptions() options { return options{ requestBuilder: DefaultRequestBuilder, @@ -14,6 +16,7 @@ func defaultOptions() options { } // WithRequestBuilder configures how authkit authorization inputs become Casbin request values. +// Pass a nil builder to leave the default in place. func WithRequestBuilder(builder RequestBuilder) Option { return func(opts *options) { if builder != nil { diff --git a/authz/casbin/request_builder_test.go b/authz/casbin/request_builder_test.go new file mode 100644 index 0000000..54289dd --- /dev/null +++ b/authz/casbin/request_builder_test.go @@ -0,0 +1,83 @@ +package casbin + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/meigma/authkit" +) + +func TestDefaultRequestBuilderProjectsClassicCasbinRequest(t *testing.T) { + tests := []struct { + name string + resource authkit.Resource + want []any + }{ + { + name: "type and ID", + resource: testResource("note", "note-1"), + want: []any{"principal_1", "note:note-1", "note:update"}, + }, + { + name: "type only", + resource: testResource("system", ""), + want: []any{"principal_1", "system", "note:update"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := DefaultRequestBuilder(testCheck("note:update", tt.resource)) + + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestDefaultRequestBuilderValidatesRequiredInputs(t *testing.T) { + tests := []struct { + name string + principal authkit.Principal + action string + resource authkit.Resource + want string + }{ + { + name: "missing principal ID", + principal: authkit.Principal{}, + action: "note:update", + resource: testResource("note", "note-1"), + want: "principal ID is required", + }, + { + name: "missing action", + principal: testPrincipal(), + action: "", + resource: testResource("note", "note-1"), + want: "action is required", + }, + { + name: "missing resource type", + principal: testPrincipal(), + action: "note:update", + resource: authkit.Resource{}, + want: "resource type is required", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := DefaultRequestBuilder(authkit.AuthorizationCheck{ + Principal: tt.principal, + Action: tt.action, + Resource: tt.resource, + }) + + require.ErrorContains(t, err, tt.want) + assert.Nil(t, got) + }) + } +} diff --git a/authz/role/authorizer.go b/authz/role/authorizer.go index c5a1f33..21ae562 100644 --- a/authz/role/authorizer.go +++ b/authz/role/authorizer.go @@ -8,9 +8,15 @@ import ( "github.com/meigma/authkit" ) +// deniedReason is the Decision.Reason returned when the principal's effective actions +// do not include the requested action. const deniedReason = "action not granted" -// Authorizer authorizes checks from effective principal actions. +// Compile-time assertion that *Authorizer satisfies the authkit.Authorizer port. +var _ authkit.Authorizer = (*Authorizer)(nil) + +// Authorizer decides authorization checks by matching check.Action against the principal's +// effective actions returned by the configured PrincipalActionResolver. type Authorizer struct { resolver authkit.PrincipalActionResolver } @@ -42,6 +48,8 @@ func (a *Authorizer) Can(ctx context.Context, check authkit.AuthorizationCheck) if err != nil { return authkit.Decision{}, err } + // Re-check after the external call so a context cancelled mid-resolution surfaces as a + // cancellation error rather than an authorization decision derived from stale state. if err := ctx.Err(); err != nil { return authkit.Decision{}, err } diff --git a/authz/role/authorizer_test.go b/authz/role/authorizer_test.go index 5aaae6e..723d24d 100644 --- a/authz/role/authorizer_test.go +++ b/authz/role/authorizer_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/meigma/authkit" @@ -16,6 +17,7 @@ import ( "github.com/meigma/authkit/authz/role" "github.com/meigma/authkit/http/auth" "github.com/meigma/authkit/internal/authtest" + authkitmocks "github.com/meigma/authkit/mocks/authkit" "github.com/meigma/authkit/store/memory" ) @@ -37,24 +39,30 @@ func TestAuthorizerCan(t *testing.T) { tests := []struct { name string - resolver *fakeActionResolver + setupMock func(resolver *authkitmocks.PrincipalActionResolver) check authkit.AuthorizationCheck assertError func(t *testing.T, err error) want authkit.Decision }{ { - name: "allows granted action", - resolver: &fakeActionResolver{actions: []string{"notes:write", testAction}}, - check: testCheck(), + name: "allows granted action", + setupMock: func(resolver *authkitmocks.PrincipalActionResolver) { + resolver.EXPECT().ResolvePrincipalActions(mock.Anything, testPrincipalID). + Return([]string{"notes:write", testAction}, nil) + }, + check: testCheck(), assertError: func(t *testing.T, err error) { require.NoError(t, err) }, want: authkit.Decision{Allowed: true}, }, { - name: "denies ungranted action", - resolver: &fakeActionResolver{actions: []string{"notes:write"}}, - check: testCheck(), + name: "denies ungranted action", + setupMock: func(resolver *authkitmocks.PrincipalActionResolver) { + resolver.EXPECT().ResolvePrincipalActions(mock.Anything, testPrincipalID). + Return([]string{"notes:write"}, nil) + }, + check: testCheck(), assertError: func(t *testing.T, err error) { require.NoError(t, err) }, @@ -64,8 +72,8 @@ func TestAuthorizerCan(t *testing.T) { }, }, { - name: "rejects missing principal ID", - resolver: &fakeActionResolver{actions: []string{testAction}}, + name: "rejects missing principal ID", + setupMock: func(_ *authkitmocks.PrincipalActionResolver) {}, check: authkit.AuthorizationCheck{ Action: testAction, }, @@ -74,8 +82,8 @@ func TestAuthorizerCan(t *testing.T) { }, }, { - name: "rejects missing action", - resolver: &fakeActionResolver{actions: []string{testAction}}, + name: "rejects missing action", + setupMock: func(_ *authkitmocks.PrincipalActionResolver) {}, check: authkit.AuthorizationCheck{ Principal: testPrincipal(), }, @@ -84,9 +92,12 @@ func TestAuthorizerCan(t *testing.T) { }, }, { - name: "propagates resolver error", - resolver: &fakeActionResolver{err: resolverErr}, - check: testCheck(), + name: "propagates resolver error", + setupMock: func(resolver *authkitmocks.PrincipalActionResolver) { + resolver.EXPECT().ResolvePrincipalActions(mock.Anything, testPrincipalID). + Return(nil, resolverErr) + }, + check: testCheck(), assertError: func(t *testing.T, err error) { require.ErrorIs(t, err, resolverErr) }, @@ -95,22 +106,22 @@ func TestAuthorizerCan(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - authorizer, err := role.NewAuthorizer(tt.resolver) + resolver := authkitmocks.NewPrincipalActionResolver(t) + tt.setupMock(resolver) + authorizer, err := role.NewAuthorizer(resolver) require.NoError(t, err) decision, err := authorizer.Can(context.Background(), tt.check) tt.assertError(t, err) assert.Equal(t, tt.want, decision) - if err == nil { - assert.Equal(t, []string{tt.check.Principal.ID}, tt.resolver.requests) - } }) } } func TestAuthorizerRespectsContextCancellation(t *testing.T) { - authorizer, err := role.NewAuthorizer(&fakeActionResolver{actions: []string{testAction}}) + resolver := authkitmocks.NewPrincipalActionResolver(t) + authorizer, err := role.NewAuthorizer(resolver) require.NoError(t, err) ctx, cancel := context.WithCancel(context.Background()) cancel() @@ -177,24 +188,6 @@ func TestAuthorizerAllowsHTTPMiddlewareThroughAccessJWT(t *testing.T) { assert.Equal(t, http.StatusNoContent, recorder.Code) } -type fakeActionResolver struct { - actions []string - requests []string - err error -} - -func (f *fakeActionResolver) ResolvePrincipalActions( - _ context.Context, - principalID string, -) ([]string, error) { - f.requests = append(f.requests, principalID) - if f.err != nil { - return nil, f.err - } - - return f.actions, nil -} - func testCheck() authkit.AuthorizationCheck { return authkit.AuthorizationCheck{ Principal: testPrincipal(), diff --git a/mocks/authkit/principal_action_resolver.go b/mocks/authkit/principal_action_resolver.go new file mode 100644 index 0000000..67930f4 --- /dev/null +++ b/mocks/authkit/principal_action_resolver.go @@ -0,0 +1,106 @@ +// Code generated by mockery; DO NOT EDIT. +// github.com/vektra/mockery +// template: testify + +package authkitmocks + +import ( + "context" + + mock "github.com/stretchr/testify/mock" +) + +// NewPrincipalActionResolver creates a new instance of PrincipalActionResolver. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewPrincipalActionResolver(t interface { + mock.TestingT + Cleanup(func()) +}) *PrincipalActionResolver { + mock := &PrincipalActionResolver{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} + +// PrincipalActionResolver is an autogenerated mock type for the PrincipalActionResolver type +type PrincipalActionResolver struct { + mock.Mock +} + +type PrincipalActionResolver_Expecter struct { + mock *mock.Mock +} + +func (_m *PrincipalActionResolver) EXPECT() *PrincipalActionResolver_Expecter { + return &PrincipalActionResolver_Expecter{mock: &_m.Mock} +} + +// ResolvePrincipalActions provides a mock function for the type PrincipalActionResolver +func (_mock *PrincipalActionResolver) ResolvePrincipalActions(ctx context.Context, principalID string) ([]string, error) { + ret := _mock.Called(ctx, principalID) + + if len(ret) == 0 { + panic("no return value specified for ResolvePrincipalActions") + } + + var r0 []string + var r1 error + if returnFunc, ok := ret.Get(0).(func(context.Context, string) ([]string, error)); ok { + return returnFunc(ctx, principalID) + } + if returnFunc, ok := ret.Get(0).(func(context.Context, string) []string); ok { + r0 = returnFunc(ctx, principalID) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]string) + } + } + if returnFunc, ok := ret.Get(1).(func(context.Context, string) error); ok { + r1 = returnFunc(ctx, principalID) + } else { + r1 = ret.Error(1) + } + return r0, r1 +} + +// PrincipalActionResolver_ResolvePrincipalActions_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'ResolvePrincipalActions' +type PrincipalActionResolver_ResolvePrincipalActions_Call struct { + *mock.Call +} + +// ResolvePrincipalActions is a helper method to define mock.On call +// - ctx context.Context +// - principalID string +func (_e *PrincipalActionResolver_Expecter) ResolvePrincipalActions(ctx interface{}, principalID interface{}) *PrincipalActionResolver_ResolvePrincipalActions_Call { + return &PrincipalActionResolver_ResolvePrincipalActions_Call{Call: _e.mock.On("ResolvePrincipalActions", ctx, principalID)} +} + +func (_c *PrincipalActionResolver_ResolvePrincipalActions_Call) Run(run func(ctx context.Context, principalID string)) *PrincipalActionResolver_ResolvePrincipalActions_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 context.Context + if args[0] != nil { + arg0 = args[0].(context.Context) + } + var arg1 string + if args[1] != nil { + arg1 = args[1].(string) + } + run( + arg0, + arg1, + ) + }) + return _c +} + +func (_c *PrincipalActionResolver_ResolvePrincipalActions_Call) Return(strings []string, err error) *PrincipalActionResolver_ResolvePrincipalActions_Call { + _c.Call.Return(strings, err) + return _c +} + +func (_c *PrincipalActionResolver_ResolvePrincipalActions_Call) RunAndReturn(run func(ctx context.Context, principalID string) ([]string, error)) *PrincipalActionResolver_ResolvePrincipalActions_Call { + _c.Call.Return(run) + return _c +}