refactor(authz): pass entire request and authentication to IsAllowed (#…

…3126) Signed-off-by: George MacRorie <[email protected]>
flipt-io · May 27, 2024 · fbd21f2 · fbd21f2
1 parent 762da37
commit fbd21f2
Show file tree

Hide file tree

Showing 8 changed files with 138 additions and 158 deletions.
diff --git a/internal/server/audit/audit_test.go b/internal/server/audit/audit_test.go
@@ -32,19 +32,28 @@ func (s *sampleSink) Close() error { return nil }
 func TestSinkSpanExporter(t *testing.T) {
 	cases := []struct {
 		name      string
-		fType     flipt.Subject
+		resource  flipt.Resource
+		subject   flipt.Subject
 		action    flipt.Action
 		expectErr bool
 	}{
 		{
 			name:      "Valid",
-			fType:     flipt.SubjectFlag,
+			resource:  flipt.ResourceFlag,
+			subject:   flipt.SubjectFlag,
+			action:    flipt.ActionCreate,
+			expectErr: false,
+		},
+		{
+			name:      "Valid Rule",
+			resource:  flipt.ResourceFlag,
+			subject:   flipt.SubjectRule,
 			action:    flipt.ActionCreate,
 			expectErr: false,
 		},
 		{
 			name:      "Invalid",
-			fType:     flipt.Subject(""),
+			subject:   flipt.Subject(""),
 			action:    flipt.Action(""),
 			expectErr: true,
 		},
@@ -68,7 +77,7 @@ func TestSinkSpanExporter(t *testing.T) {
 
 			_, span := tr.Start(ctx, "OnStart")
 
-			r := flipt.NewRequest(c.fType, c.action)
+			r := flipt.NewRequest(c.resource, c.action, flipt.WithSubject(c.subject))
 			e := NewEvent(
 				r,
 				&Actor{

diff --git a/internal/server/audit/events.go b/internal/server/audit/events.go
@@ -51,10 +51,15 @@ func NewEvent(r flipt.Request, actor *Actor, payload interface{}) *Event {
 		action = "read"
 	}
 
+	typ := string(r.Resource)
+	if r.Subject != "" {
+		typ = string(r.Subject)
+	}
+
 	return &Event{
 		Version: eventVersion,
 		Action:  action,
-		Type:    string(r.Subject),
+		Type:    typ,
 		Metadata: Metadata{
 			Actor: actor,
 		},

diff --git a/internal/server/audit/logfile/logfile_test.go b/internal/server/audit/logfile/logfile_test.go
@@ -208,7 +208,7 @@ func TestSink_SendAudits(t *testing.T) {
 	require.NoError(t, err)
 	require.NotNil(t, sink)
 
-	e := audit.NewEvent(flipt.NewRequest(flipt.SubjectFlag, flipt.ActionCreate), nil, nil)
+	e := audit.NewEvent(flipt.NewRequest(flipt.ResourceFlag, flipt.ActionCreate), nil, nil)
 	assert.NoError(t, sink.SendAudits(context.Background(), []audit.Event{*e}))
 
 	assert.NotNil(t, f.Buffer)

diff --git a/internal/server/authn/server.go b/internal/server/authn/server.go
@@ -180,7 +180,7 @@ func (s *Server) DeleteAuthentication(ctx context.Context, req *auth.DeleteAuthe
 			return nil, err
 		}
 		if a.Method == auth.Method_METHOD_TOKEN {
-			request := flipt.NewRequest(flipt.SubjectToken, flipt.ActionDelete)
+			request := flipt.NewRequest(flipt.ResourceAuthentication, flipt.ActionDelete, flipt.WithSubject(flipt.SubjectToken))
 			event := audit.NewEvent(request, actor, a.Metadata)
 			event.AddToSpan(ctx)
 		}

diff --git a/internal/server/authz/middleware/grpc/middleware.go b/internal/server/authz/middleware/grpc/middleware.go
@@ -73,34 +73,9 @@ func AuthorizationRequiredInterceptor(logger *zap.Logger, policyVerifier authz.V
 			return ctx, errUnauthorized
 		}
 
-		role, ok := auth.Metadata["io.flipt.auth.role"]
-		if !ok {
-			logger.Error("unauthorized", zap.String("reason", "user role required"))
-			return ctx, errUnauthorized
-		}
-
-		request := requester.Request()
-		action := string(request.Action)
-		if action == "" {
-			logger.Error("unauthorized", zap.String("reason", "request action required"))
-			return ctx, errUnauthorized
-		}
-
-		resource := string(request.Resource)
-		if resource == "" {
-			// fallback to subject if resource is not provided
-			resource = string(request.Subject)
-		}
-
-		if resource == "" {
-			logger.Error("unauthorized", zap.String("reason", "request resource or subject required"))
-			return ctx, errUnauthorized
-		}
-
 		allowed, err := policyVerifier.IsAllowed(ctx, map[string]interface{}{
-			"role":     role,
-			"action":   action,
-			"resource": resource,
+			"request":        requester.Request(),
+			"authentication": auth,
 		})
 
 		if err != nil {

diff --git a/internal/server/authz/middleware/grpc/middleware_test.go b/internal/server/authz/middleware/grpc/middleware_test.go
@@ -5,6 +5,7 @@ import (
 	"errors"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	authmiddlewaregrpc "go.flipt.io/flipt/internal/server/authn/middleware/grpc"
 	"go.flipt.io/flipt/rpc/flipt"
@@ -16,9 +17,11 @@ import (
 type mockPolicyVerifier struct {
 	isAllowed bool
 	wantErr   error
+	input     map[string]any
 }
 
-func (v *mockPolicyVerifier) IsAllowed(ctx context.Context, input map[string]interface{}) (bool, error) {
+func (v *mockPolicyVerifier) IsAllowed(ctx context.Context, input map[string]any) (bool, error) {
+	v.input = input
 	return v.isAllowed, v.wantErr
 }
 
@@ -31,17 +34,13 @@ func (s *mockServer) SkipsAuthorization(ctx context.Context) bool {
 	return s.skipsAuthz
 }
 
-type mockRequester struct {
-	action   flipt.Action
-	resource flipt.Resource
-}
-
-func (r *mockRequester) Request() flipt.Request {
-	return flipt.Request{
-		Action:   r.action,
-		Resource: r.resource,
+var (
+	adminAuth = &authrpc.Authentication{
+		Metadata: map[string]string{
+			"io.flipt.auth.role": "admin",
+		},
 	}
-}
+)
 
 func TestAuthorizationRequiredInterceptor(t *testing.T) {
 	var tests = []struct {
@@ -52,28 +51,45 @@ func TestAuthorizationRequiredInterceptor(t *testing.T) {
 		validatorAllowed bool
 		validatorErr     error
 		wantAllowed      bool
+		authzInput       map[string]any
 	}{
 		{
-			name: "allowed",
-			authn: &authrpc.Authentication{
-				Metadata: map[string]string{
-					"io.flipt.auth.role": "admin",
-				},
+			name:  "allowed",
+			authn: adminAuth,
+			req: &flipt.CreateFlagRequest{
+				NamespaceKey: "default",
+				Key:          "some_flag",
 			},
-			req:              &flipt.CreateFlagRequest{},
 			validatorAllowed: true,
 			wantAllowed:      true,
+			authzInput: map[string]any{
+				"request": flipt.Request{
+					Namespace: "default",
+					Resource:  flipt.ResourceFlag,
+					Subject:   flipt.SubjectFlag,
+					Action:    flipt.ActionCreate,
+				},
+				"authentication": adminAuth,
+			},
 		},
 		{
-			name: "not allowed",
-			authn: &authrpc.Authentication{
-				Metadata: map[string]string{
-					"io.flipt.auth.role": "admin",
-				},
+			name:  "not allowed",
+			authn: adminAuth,
+			req: &flipt.CreateFlagRequest{
+				NamespaceKey: "default",
+				Key:          "some_other_flag",
 			},
-			req:              &flipt.CreateFlagRequest{},
 			validatorAllowed: false,
 			wantAllowed:      false,
+			authzInput: map[string]any{
+				"request": flipt.Request{
+					Namespace: "default",
+					Resource:  flipt.ResourceFlag,
+					Subject:   flipt.SubjectFlag,
+					Action:    flipt.ActionCreate,
+				},
+				"authentication": adminAuth,
+			},
 		},
 		{
 			name: "skips authz",
@@ -89,54 +105,14 @@ func TestAuthorizationRequiredInterceptor(t *testing.T) {
 			wantAllowed: false,
 		},
 		{
-			name: "no role",
-			authn: &authrpc.Authentication{
-				Metadata: map[string]string{},
-			},
-			req:         &flipt.CreateFlagRequest{},
-			wantAllowed: false,
-		},
-		{
-			name: "invalid request",
-			authn: &authrpc.Authentication{
-				Metadata: map[string]string{
-					"io.flipt.auth.role": "admin",
-				},
-			},
+			name:        "invalid request",
+			authn:       adminAuth,
 			req:         struct{}{},
 			wantAllowed: false,
 		},
 		{
-			name: "missing action",
-			authn: &authrpc.Authentication{
-				Metadata: map[string]string{
-					"io.flipt.auth.role": "admin",
-				},
-			},
-			req: &mockRequester{
-				resource: "foo",
-			},
-			wantAllowed: false,
-		},
-		{
-			name: "missing resource",
-			authn: &authrpc.Authentication{
-				Metadata: map[string]string{
-					"io.flipt.auth.role": "admin",
-				},
-			},
-			req: &mockRequester{
-				action: "action",
-			},
-			wantAllowed: false,
-		},
-		{
-			name: "validator error",
-			authn: &authrpc.Authentication{
-				Metadata: map[string]string{
-					"io.flipt.auth.role": "admin",
-				},
-			},
+			name:         "validator error",
+			authn:        adminAuth,
 			req:          &flipt.CreateFlagRequest{},
 			validatorErr: errors.New("error"),
 			wantAllowed:  false,
@@ -157,7 +133,10 @@ func TestAuthorizationRequiredInterceptor(t *testing.T) {
 				}
 
 				srv           = &grpc.UnaryServerInfo{Server: &mockServer{}}
-				policyVerfier = &mockPolicyVerifier{isAllowed: tt.validatorAllowed, wantErr: tt.validatorErr}
+				policyVerfier = &mockPolicyVerifier{
+					isAllowed: tt.validatorAllowed,
+					wantErr:   tt.validatorErr,
+				}
 			)
 
 			if tt.server != nil {
@@ -170,6 +149,7 @@ func TestAuthorizationRequiredInterceptor(t *testing.T) {
 
 			if tt.wantAllowed {
 				require.NoError(t, err)
+				assert.Equal(t, tt.authzInput, policyVerfier.input)
 				return
 			}
 

diff --git a/rpc/flipt/auth/request.go b/rpc/flipt/auth/request.go
@@ -5,5 +5,5 @@ import (
 )
 
 func (req *CreateTokenRequest) Request() flipt.Request {
-	return flipt.NewRequest(flipt.SubjectToken, flipt.ActionCreate)
+	return flipt.NewRequest(flipt.ResourceAuthentication, flipt.ActionCreate, flipt.WithSubject(flipt.SubjectToken))
 }