refactor(authz): readability + maintainability sweep

.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

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
 	}

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
-		},
-	}
-}

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
+		},
+	}
+}

authz/casbin/options.go

@@ -1,19 +1,22 @@
 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,
 	}
 }
 
 // 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 {