Skip to content

Commit fe35517

Browse files
committed
Merge remote-tracking branch 'origin' into release/v1.51.0
2 parents 848dfda + 3bbc7d4 commit fe35517

File tree

5 files changed

+4
-259
lines changed

5 files changed

+4
-259
lines changed

docs/advanced-guide/rbac/page.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -413,7 +413,7 @@ Or use role inheritance to avoid duplication:
413413
- **Never use header-based RBAC for public APIs** - Use JWT-based RBAC
414414
- **Always validate JWT tokens** - Use proper JWKS endpoints with HTTPS
415415
- **Use HTTPS in production** - Protect tokens and headers
416-
- **Monitor audit logs** - Track authorization decisions
416+
- **Monitor logs** - Track authorization decisions
417417

418418
### Configuration
419419
- **Use role inheritance** - Avoid duplicating permissions (only specify additional ones)
@@ -438,13 +438,13 @@ Or use role inheritance to avoid duplication:
438438
**Permission always denied**
439439
- Check role assignment - verify user's role has the required permission
440440
- Review role permissions - ensure `roles[].permissions` includes the required permission
441-
- Enable debug logging - check audit logs for authorization decisions
441+
- Enable debug logging - check debug logs for authorization decisions
442442

443443
**Permission always allowed**
444444
- Check if endpoint is in RBAC config - routes not in config are allowed to proceed
445445
- Check public endpoints - verify endpoint is not marked as `public: true`
446446
- Review endpoint configuration - ensure `endpoints[].requiredPermissions` is set correctly
447-
- Verify permission check - check audit logs to see if permission check is being performed
447+
- Verify permission check - check logs to see if permission check is being performed
448448

449449
**JWT role extraction failing**
450450
- Ensure OAuth middleware is enabled before RBAC
@@ -519,7 +519,7 @@ RBAC middleware implements industry-standard security practices to protect sensi
519519
- ✅ Status (allowed/denied) included
520520
- ❌ Roles excluded (avoid high cardinality and PII concerns)
521521

522-
**Audit Logs:**
522+
**Logs:**
523523
- ✅ Roles included (required for compliance: SOC 2, PCI-DSS, NIST)
524524
- ✅ HTTP method, route, status, and reason included
525525
- ❌ No authorization tokens, headers, or request bodies logged

pkg/gofr/rbac/config.go

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -163,11 +163,6 @@ func LoadPermissions(path string, logger datasource.Logger, metrics container.Me
163163
// Use StrictSlash(false) to match the application router's behavior
164164
config.muxRouter = mux.NewRouter().StrictSlash(false)
165165

166-
// Register metrics if provided
167-
if metrics != nil {
168-
registerMetrics(metrics)
169-
}
170-
171166
// Validate config before processing
172167
if err := config.validate(); err != nil {
173168
return nil, fmt.Errorf("invalid RBAC config: %w", err)
@@ -181,18 +176,6 @@ func LoadPermissions(path string, logger datasource.Logger, metrics container.Me
181176
return &config, nil
182177
}
183178

184-
// registerMetrics registers RBAC metrics with the metrics instance.
185-
func registerMetrics(metrics container.Metrics) {
186-
buckets := []float64{0.0001, 0.0005, 0.001, 0.005, 0.01, 0.05, 0.1, 0.5, 1}
187-
metrics.NewHistogram(
188-
"rbac_authorization_duration",
189-
"Duration of RBAC authorization checks",
190-
buckets...,
191-
)
192-
metrics.NewCounter("rbac_authorization_decisions", "Number of RBAC authorization decisions")
193-
metrics.NewCounter("rbac_role_extraction_failures", "Number of failed role extractions")
194-
}
195-
196179
// validate validates the RBAC configuration.
197180
func (c *Config) validate() error {
198181
// Validate endpoints: non-public endpoints must have RequiredPermissions

pkg/gofr/rbac/config_test.go

Lines changed: 0 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package rbac
22

33
import (
4-
"context"
54
"os"
65
"path/filepath"
76
"testing"
@@ -538,50 +537,6 @@ func TestConfig_MatchesKey(t *testing.T) {
538537
})
539538
}
540539

541-
func TestLoadPermissions_RegisterMetrics(t *testing.T) {
542-
t.Run("registers metrics when metrics is provided", func(t *testing.T) {
543-
mockMetrics := &configMockMetrics{
544-
histogramCreated: false,
545-
counterCreated: false,
546-
}
547-
548-
fileContent := `{
549-
"roles": [{"name": "admin", "permissions": ["admin:read"]}],
550-
"endpoints": [{"path": "/api", "methods": ["GET"], "requiredPermissions": ["admin:read"]}]
551-
}`
552-
path, err := createTestConfigFile("test_metrics.json", fileContent)
553-
require.NoError(t, err)
554-
555-
defer os.Remove(path)
556-
557-
_, err = LoadPermissions(path, nil, mockMetrics, nil)
558-
require.NoError(t, err)
559-
560-
assert.True(t, mockMetrics.histogramCreated, "histogram should be created")
561-
assert.True(t, mockMetrics.counterCreated, "counter should be created")
562-
})
563-
}
564-
565-
// configMockMetrics for config tests.
566-
type configMockMetrics struct {
567-
histogramCreated bool
568-
counterCreated bool
569-
}
570-
571-
func (m *configMockMetrics) NewHistogram(_, _ string, _ ...float64) {
572-
m.histogramCreated = true
573-
}
574-
575-
func (*configMockMetrics) RecordHistogram(_ context.Context, _ string, _ float64, _ ...string) {}
576-
func (m *configMockMetrics) NewCounter(_, _ string) {
577-
m.counterCreated = true
578-
}
579-
func (*configMockMetrics) IncrementCounter(_ context.Context, _ string, _ ...string) {}
580-
func (*configMockMetrics) NewUpDownCounter(_, _ string) {}
581-
func (*configMockMetrics) NewGauge(_, _ string) {}
582-
func (*configMockMetrics) DeltaUpDownCounter(_ context.Context, _ string, _ float64, _ ...string) {}
583-
func (*configMockMetrics) SetGauge(_ string, _ float64, _ ...string) {}
584-
585540
func TestConfig_getEffectivePermissions(t *testing.T) {
586541
testCases := []struct {
587542
desc string

pkg/gofr/rbac/middleware.go

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -144,24 +144,11 @@ func Middleware(config *Config) func(handler http.Handler) http.Handler {
144144
// Check authorization using unified endpoint-based authorization
145145
authorized, _ := checkEndpointAuthorization(role, endpoint, config)
146146
if !authorized {
147-
// Role not included in metrics to avoid high cardinality and PII concerns
148-
// Only include status (allowed/denied) for aggregation
149-
if config.Metrics != nil {
150-
config.Metrics.IncrementCounter(r.Context(), "rbac_authorization_decisions", "status", "denied")
151-
}
152-
153147
handleAuthError(w, r, config, role, routeLabel, ErrAccessDenied)
154148

155149
return
156150
}
157151

158-
// Record metrics and update span
159-
// Role not included in metrics to avoid high cardinality and PII concerns
160-
// Only include status (allowed/denied) for aggregation
161-
if config.Metrics != nil {
162-
config.Metrics.IncrementCounter(r.Context(), "rbac_authorization_decisions", "status", "allowed")
163-
}
164-
165152
if config.Tracer != nil {
166153
trace.SpanFromContext(r.Context()).SetAttributes(attribute.Bool("rbac.authorized", true))
167154
}

pkg/gofr/rbac/middleware_test.go

Lines changed: 0 additions & 180 deletions
Original file line numberDiff line numberDiff line change
@@ -923,186 +923,6 @@ func TestMiddleware_WithTracing(t *testing.T) {
923923
})
924924
}
925925

926-
func TestMiddleware_WithMetrics(t *testing.T) {
927-
t.Run("records metrics when metrics is available", func(t *testing.T) {
928-
mockMetrics := &mockMetrics{
929-
counterIncremented: make(map[string]bool),
930-
}
931-
932-
config := &Config{
933-
Endpoints: []EndpointMapping{
934-
{Path: "/api", Methods: []string{"GET"}, RequiredPermissions: []string{"admin:read"}},
935-
},
936-
RoleHeader: "X-User-Role",
937-
Metrics: mockMetrics,
938-
}
939-
err := config.processUnifiedConfig()
940-
require.NoError(t, err)
941-
942-
middlewareFunc := Middleware(config)
943-
handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
944-
w.WriteHeader(http.StatusOK)
945-
})
946-
947-
wrapped := middlewareFunc(handler)
948-
w := httptest.NewRecorder()
949-
req := httptest.NewRequest(http.MethodGet, "/api", http.NoBody)
950-
req.Header.Set("X-User-Role", "admin")
951-
952-
// Setup role permissions
953-
config.rolePermissionsMap = map[string][]string{
954-
"admin": {"admin:read"},
955-
}
956-
957-
wrapped.ServeHTTP(w, req)
958-
959-
assert.Equal(t, http.StatusOK, w.Code)
960-
// Metrics should be recorded
961-
assert.True(t, mockMetrics.counterIncremented["rbac_authorization_decisions"])
962-
})
963-
}
964-
965-
// mockMetrics implements the Metrics interface for testing.
966-
type mockMetrics struct {
967-
histogramCreated bool
968-
counterCreated bool
969-
counterIncremented map[string]bool
970-
counterLabels map[string][]string // Track labels for each counter call
971-
}
972-
973-
func (m *mockMetrics) NewHistogram(_, _ string, _ ...float64) {
974-
m.histogramCreated = true
975-
}
976-
977-
func (*mockMetrics) RecordHistogram(_ context.Context, _ string, _ float64, _ ...string) {
978-
// Mock implementation
979-
}
980-
981-
func (m *mockMetrics) NewCounter(_, _ string) {
982-
m.counterCreated = true
983-
}
984-
985-
func (m *mockMetrics) IncrementCounter(_ context.Context, name string, labels ...string) {
986-
if m.counterIncremented == nil {
987-
m.counterIncremented = make(map[string]bool)
988-
}
989-
990-
if m.counterLabels == nil {
991-
m.counterLabels = make(map[string][]string)
992-
}
993-
994-
m.counterIncremented[name] = true
995-
m.counterLabels[name] = labels
996-
}
997-
998-
func (*mockMetrics) NewUpDownCounter(_, _ string) {
999-
// Mock implementation
1000-
}
1001-
1002-
func (*mockMetrics) NewGauge(_, _ string) {
1003-
// Mock implementation
1004-
}
1005-
1006-
func (*mockMetrics) DeltaUpDownCounter(_ context.Context, _ string, _ float64, _ ...string) {
1007-
// Mock implementation
1008-
}
1009-
1010-
func (*mockMetrics) SetGauge(_ string, _ float64, _ ...string) {
1011-
// Mock implementation
1012-
}
1013-
1014-
func TestMiddleware_RoleNotInMetrics(t *testing.T) {
1015-
t.Run("role is not included in metric labels", func(t *testing.T) {
1016-
mockMetrics := &mockMetrics{
1017-
counterIncremented: make(map[string]bool),
1018-
counterLabels: make(map[string][]string),
1019-
}
1020-
1021-
config := &Config{
1022-
Endpoints: []EndpointMapping{
1023-
{Path: "/api", Methods: []string{"GET"}, RequiredPermissions: []string{"admin:read"}},
1024-
},
1025-
RoleHeader: "X-User-Role",
1026-
Metrics: mockMetrics,
1027-
}
1028-
err := config.processUnifiedConfig()
1029-
require.NoError(t, err)
1030-
1031-
middlewareFunc := Middleware(config)
1032-
handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
1033-
w.WriteHeader(http.StatusOK)
1034-
})
1035-
1036-
wrapped := middlewareFunc(handler)
1037-
w := httptest.NewRecorder()
1038-
req := httptest.NewRequest(http.MethodGet, "/api", http.NoBody)
1039-
req.Header.Set("X-User-Role", "admin")
1040-
1041-
// Setup role permissions
1042-
config.rolePermissionsMap = map[string][]string{
1043-
"admin": {"admin:read"},
1044-
}
1045-
1046-
wrapped.ServeHTTP(w, req)
1047-
1048-
assert.Equal(t, http.StatusOK, w.Code)
1049-
// Verify metrics were recorded
1050-
assert.True(t, mockMetrics.counterIncremented["rbac_authorization_decisions"])
1051-
// Verify role is NOT in labels (only status should be present)
1052-
labels := mockMetrics.counterLabels["rbac_authorization_decisions"]
1053-
assert.Contains(t, labels, "status", "status label should be present")
1054-
assert.Contains(t, labels, "allowed", "allowed status should be present")
1055-
// Verify role is NOT in labels
1056-
assert.NotContains(t, labels, "role", "role should not be in metric labels")
1057-
assert.NotContains(t, labels, "admin", "role value should not be in metric labels")
1058-
})
1059-
1060-
t.Run("role is not included in metric labels for denied requests", func(t *testing.T) {
1061-
mockMetrics := &mockMetrics{
1062-
counterIncremented: make(map[string]bool),
1063-
counterLabels: make(map[string][]string),
1064-
}
1065-
1066-
config := &Config{
1067-
Endpoints: []EndpointMapping{
1068-
{Path: "/api", Methods: []string{"GET"}, RequiredPermissions: []string{"admin:read"}},
1069-
},
1070-
RoleHeader: "X-User-Role",
1071-
Metrics: mockMetrics,
1072-
}
1073-
err := config.processUnifiedConfig()
1074-
require.NoError(t, err)
1075-
1076-
middlewareFunc := Middleware(config)
1077-
handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
1078-
w.WriteHeader(http.StatusOK)
1079-
})
1080-
1081-
wrapped := middlewareFunc(handler)
1082-
w := httptest.NewRecorder()
1083-
req := httptest.NewRequest(http.MethodGet, "/api", http.NoBody)
1084-
req.Header.Set("X-User-Role", "viewer") // Role without permission
1085-
1086-
// Setup role permissions
1087-
config.rolePermissionsMap = map[string][]string{
1088-
"viewer": {"viewer:read"}, // Different permission
1089-
}
1090-
1091-
wrapped.ServeHTTP(w, req)
1092-
1093-
assert.Equal(t, http.StatusForbidden, w.Code)
1094-
// Verify metrics were recorded
1095-
assert.True(t, mockMetrics.counterIncremented["rbac_authorization_decisions"])
1096-
// Verify role is NOT in labels (only status should be present)
1097-
labels := mockMetrics.counterLabels["rbac_authorization_decisions"]
1098-
assert.Contains(t, labels, "status", "status label should be present")
1099-
assert.Contains(t, labels, "denied", "denied status should be present")
1100-
// Verify role is NOT in labels
1101-
assert.NotContains(t, labels, "role", "role should not be in metric labels")
1102-
assert.NotContains(t, labels, "viewer", "role value should not be in metric labels")
1103-
})
1104-
}
1105-
1106926
func TestMiddleware_RoleInAuditLogs(t *testing.T) {
1107927
t.Run("role is included in audit logs", func(t *testing.T) {
1108928
mockLog := &mockLogger{

0 commit comments

Comments
 (0)