Authz OIDC tests (#3098)

* chore: fix tests, add role attribute path / role mapping to oidc server tests Signed-off-by: Mark Phelps <[email protected]> * chore: authz middleware tests Signed-off-by: Mark Phelps <[email protected]> * chore: fix audit tests Signed-off-by: Mark Phelps <[email protected]> * chore: proto regen Signed-off-by: Mark Phelps <[email protected]> * chore: try to fix marshal audit events behaviour Signed-off-by: Mark Phelps <[email protected]> * chore: fix failing test Signed-off-by: Mark Phelps <[email protected]> --------- Signed-off-by: Mark Phelps <[email protected]>
flipt-io · May 21, 2024 · ab1b48a · ab1b48a
1 parent f4e8290
commit ab1b48a
Show file tree

Hide file tree

Showing 24 changed files with 307 additions and 81 deletions.
diff --git a/go.work.sum b/go.work.sum
@@ -509,7 +509,9 @@ github.com/go-openapi/swag v0.19.2/go.mod h1:POnQmlKehdgb5mhVOsnJFsivZCEZ/vjK9gh
 github.com/go-openapi/swag v0.19.5/go.mod h1:POnQmlKehdgb5mhVOsnJFsivZCEZ/vjK9gh66Z9tfKk=
 github.com/go-openapi/swag v0.19.14/go.mod h1:QYRuS/SOXUCsnplDa677K7+DxSOj6IPNl/eQntq43wQ=
 github.com/go-openapi/swag v0.22.3/go.mod h1:UzaqsxGiab7freDnrUUra0MwWfN/q7tE4j+VcZ0yl14=
+github.com/go-task/slim-sprig v0.0.0-20230315185526-52ccab3ef572/go.mod h1:9Pwr4B2jHnOSGXyyzV8ROjYa2ojvAY6HCGYYfMoC3Ls=
 github.com/gobuffalo/here v0.6.0/go.mod h1:wAG085dHOYqUpf+Ap+WOdrPTp5IYcDAs/x7PLa8Y5fM=
+github.com/goccy/go-json v0.9.11/go.mod h1:6MelG93GURQebXPDq3khkgXZkazVtN9CRI+MGFi0w8I=
 github.com/goccy/go-json v0.10.2/go.mod h1:6MelG93GURQebXPDq3khkgXZkazVtN9CRI+MGFi0w8I=
 github.com/gocql/gocql v0.0.0-20210515062232-b7ef815b4556/go.mod h1:DL0ekTmBSTdlNF25Orwt/JMzqIq3EJ4MVa/J/uK64OY=
 github.com/godbus/dbus v0.0.0-20151105175453-c7fdd8b5cd55/go.mod h1:/YcGZj5zSblfDWMMoOzV4fas9FZnQYTkDnsGvmh2Grw=

diff --git a/internal/server/audit/checker_test.go b/internal/server/audit/checker_test.go
@@ -121,15 +121,17 @@ func TestChecker(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		checker, err := NewChecker(tc.eventPairs)
-		if tc.expectedError != nil {
-			assert.EqualError(t, err, tc.expectedError.Error())
-			continue
-		}
+		t.Run(tc.name, func(t *testing.T) {
+			checker, err := NewChecker(tc.eventPairs)
+			if tc.expectedError != nil {
+				assert.EqualError(t, err, tc.expectedError.Error())
+				return
+			}
 
-		for k, v := range tc.pairs {
-			actual := checker.Check(k)
-			assert.Equal(t, v, actual)
-		}
+			for k, v := range tc.pairs {
+				actual := checker.Check(k)
+				assert.Equal(t, v, actual)
+			}
+		})
 	}
 }
diff --git a/internal/server/audit/cloud/cloud_test.go b/internal/server/audit/cloud/cloud_test.go
@@ -6,6 +6,7 @@ import (
 
 	"github.com/stretchr/testify/require"
 	"go.flipt.io/flipt/internal/server/audit"
+	"go.flipt.io/flipt/rpc/flipt"
 	"go.uber.org/zap"
 )
 
@@ -26,13 +27,13 @@ func TestSink(t *testing.T) {
 	err := s.SendAudits(context.TODO(), []audit.Event{
 		{
 			Version: "0.1",
-			Type:    audit.FlagType,
-			Action:  audit.Create,
+			Type:    flipt.SubjectFlag,
+			Action:  flipt.ActionCreate,
 		},
 		{
 			Version: "0.1",
-			Type:    audit.ConstraintType,
-			Action:  audit.Update,
+			Type:    flipt.SubjectConstraint,
+			Action:  flipt.ActionUpdate,
 		},
 	})
 

diff --git a/internal/server/audit/events.go b/internal/server/audit/events.go
@@ -27,7 +27,7 @@ type Event struct {
 
 	Type flipt.Subject `json:"type"`
 
-	Action flipt.Action `json:"action"`
+	Action flipt.Action `json:"-"`
 
 	Metadata Metadata `json:"metadata"`
 
@@ -36,6 +36,20 @@ type Event struct {
 	Timestamp string `json:"timestamp"`
 }
 
+func (e Event) MarshalJSON() ([]byte, error) {
+
+	type EventAlias Event
+
+	return json.Marshal(&struct {
+		*EventAlias
+		Action string `json:"action"`
+	}{
+		EventAlias: (*EventAlias)(&e),
+		Action:     e.Action.Past(),
+	})
+
+}
+
 // NewEvent is the constructor for an event.
 func NewEvent(r flipt.Request, actor *Actor, payload interface{}) *Event {
 	return &Event{

diff --git a/internal/server/audit/logfile/logfile_test.go b/internal/server/audit/logfile/logfile_test.go
@@ -215,7 +215,7 @@ func TestSink_SendAudits(t *testing.T) {
 		},
 	}))
 
-	assert.Equal(t, `{"version":"1","type":"flag","action":"created","metadata":{},"payload":null,"timestamp":""}
+	assert.JSONEq(t, `{"version":"1","type":"flag","action":"created","metadata":{},"payload":null,"timestamp":""}
 `, f.Buffer.String())
 	assert.NoError(t, sink.Close())
 }
diff --git a/internal/server/audit/template/executer_test.go b/internal/server/audit/template/executer_test.go
@@ -48,7 +48,7 @@ func TestExecuter_JSON_Failure(t *testing.T) {
 		Action: flipt.ActionCreate,
 	})
 
-	assert.EqualError(t, err, "invalid JSON: this is invalid JSON flag, created")
+	assert.EqualError(t, err, "invalid JSON: this is invalid JSON flag, create")
 }
 
 func TestExecuter_Execute(t *testing.T) {

diff --git a/internal/server/authn/method/oidc/server_test.go b/internal/server/authn/method/oidc/server_test.go
@@ -65,6 +65,7 @@ func Test_Server_ImplicitFlow(t *testing.T) {
 				CustomClaims: map[string]interface{}{
 					"email": "[email protected]",
 					"name":  "Mark Phelps",
+					"roles": []string{"admin"},
 				},
 			},
 			"george": {
@@ -76,6 +77,7 @@ func Test_Server_ImplicitFlow(t *testing.T) {
 				CustomClaims: map[string]interface{}{
 					"email": "[email protected]",
 					"name":  "George MacRorie",
+					"roles": []string{"editor"},
 				},
 			},
 		},
@@ -107,10 +109,11 @@ func Test_Server_ImplicitFlow(t *testing.T) {
 					Method: config.AuthenticationMethodOIDCConfig{
 						Providers: map[string]config.AuthenticationMethodOIDCProvider{
 							"google": {
-								IssuerURL:       tp.Addr(),
-								ClientID:        id,
-								ClientSecret:    secret,
-								RedirectAddress: clientAddress,
+								IssuerURL:         tp.Addr(),
+								ClientID:          id,
+								ClientSecret:      secret,
+								RedirectAddress:   clientAddress,
+								RoleAttributePath: "contains(roles[*], 'admin') && 'admin' || contains(roles[*], 'editor') && 'editor' || 'viewer'",
 							},
 						},
 					},
@@ -167,6 +170,7 @@ func Test_Server_PKCE(t *testing.T) {
 				CustomClaims: map[string]interface{}{
 					"email": "[email protected]",
 					"name":  "Mark Phelps",
+					"roles": []string{"admin"},
 				},
 			},
 			"george": {
@@ -178,6 +182,7 @@ func Test_Server_PKCE(t *testing.T) {
 				CustomClaims: map[string]interface{}{
 					"email": "[email protected]",
 					"name":  "George MacRorie",
+					"roles": []string{"editor"},
 				},
 			},
 		},
@@ -210,11 +215,12 @@ func Test_Server_PKCE(t *testing.T) {
 					Method: config.AuthenticationMethodOIDCConfig{
 						Providers: map[string]config.AuthenticationMethodOIDCProvider{
 							"google": {
-								IssuerURL:       tp.Addr(),
-								ClientID:        id,
-								ClientSecret:    secret,
-								RedirectAddress: clientAddress,
-								UsePKCE:         true,
+								IssuerURL:         tp.Addr(),
+								ClientID:          id,
+								ClientSecret:      secret,
+								RedirectAddress:   clientAddress,
+								UsePKCE:           true,
+								RoleAttributePath: "contains(roles[*], 'admin') && 'admin' || contains(roles[*], 'editor') && 'editor' || 'viewer'",
 							},
 						},
 					},
@@ -345,6 +351,7 @@ func testOIDCFlow(t *testing.T, ctx context.Context, tpAddr, clientAddress strin
 			"io.flipt.auth.oidc.email":    "[email protected]",
 			"io.flipt.auth.oidc.name":     "Mark Phelps",
 			"io.flipt.auth.oidc.sub":      "mark",
+			"io.flipt.auth.role":          "admin",
 		}, response.Authentication.Metadata)
 
 		// ensure expiry is set

diff --git a/internal/server/authn/middleware/grpc/middleware_test.go b/internal/server/authn/middleware/grpc/middleware_test.go
@@ -25,18 +25,18 @@ import (
 	"google.golang.org/protobuf/types/known/timestamppb"
 )
 
-// mockServer is used to test skipping auth
+// mockServer is used to test skipping authn
 type mockServer struct {
-	skipsAuth           bool
-	allowNamespacedAuth bool
+	skipsAuthn           bool
+	allowNamespacedAuthn bool
 }
 
 func (s *mockServer) SkipsAuthentication(ctx context.Context) bool {
-	return s.skipsAuth
+	return s.skipsAuthn
 }
 
 func (s *mockServer) AllowsNamespaceScopedAuthentication(ctx context.Context) bool {
-	return s.allowNamespacedAuth
+	return s.allowNamespacedAuthn
 }
 
 var priv *rsa.PrivateKey
@@ -186,7 +186,7 @@ func TestJWTAuthenticationInterceptor(t *testing.T) {
 		{
 			name: "successful authentication (skipped)",
 			server: &mockServer{
-				skipsAuth: true,
+				skipsAuthn: true,
 			},
 		},
 		{
@@ -345,7 +345,7 @@ func TestClientTokenAuthenticationInterceptor(t *testing.T) {
 			name:     "successful authentication (skipped)",
 			metadata: metadata.MD{},
 			server: &mockServer{
-				skipsAuth: true,
+				skipsAuthn: true,
 			},
 		},
 		{
@@ -675,7 +675,7 @@ func TestNamespaceMatchingInterceptor(t *testing.T) {
 			},
 			req: &struct{}{},
 			srv: &mockServer{
-				skipsAuth: true,
+				skipsAuthn: true,
 			},
 			wantCalled: true,
 		},
@@ -793,7 +793,7 @@ func TestNamespaceMatchingInterceptor(t *testing.T) {
 				NamespaceKey: "foo",
 			},
 			srv: &mockServer{
-				allowNamespacedAuth: false,
+				allowNamespacedAuthn: false,
 			},
 			expectedErr: ErrUnauthenticated,
 		},
@@ -820,7 +820,7 @@ func TestNamespaceMatchingInterceptor(t *testing.T) {
 				}
 
 				srv = &grpc.UnaryServerInfo{Server: &mockServer{
-					allowNamespacedAuth: true,
+					allowNamespacedAuthn: true,
 				}}
 			)
 

diff --git a/internal/server/authz/engine.go b/internal/server/authz/engine.go
@@ -9,6 +9,12 @@ import (
 	"github.com/open-policy-agent/opa/storage/inmem"
 )
 
+var _ Verifier = &Engine{}
+
+type Verifier interface {
+	IsAllowed(ctx context.Context, input map[string]interface{}) (bool, error)
+}
+
 type Engine struct {
 	query rego.PreparedEvalQuery
 }

diff --git a/internal/server/authz/middleware/grpc/middleware.go b/internal/server/authz/middleware/grpc/middleware.go
@@ -50,7 +50,7 @@ func WithServerSkipsAuthorization(server any) containers.Option[InterceptorOptio
 	}
 }
 
-func AuthorizationRequiredInterceptor(logger *zap.Logger, policyEngine *authz.Engine, o ...containers.Option[InterceptorOptions]) grpc.UnaryServerInterceptor {
+func AuthorizationRequiredInterceptor(logger *zap.Logger, policyVerifier authz.Verifier, o ...containers.Option[InterceptorOptions]) grpc.UnaryServerInterceptor {
 	var opts InterceptorOptions
 	containers.ApplyAll(&opts, o...)
 
@@ -92,7 +92,7 @@ func AuthorizationRequiredInterceptor(logger *zap.Logger, policyEngine *authz.En
 			return ctx, errUnauthorized
 		}
 
-		allowed, err := policyEngine.IsAllowed(ctx, map[string]interface{}{
+		allowed, err := policyVerifier.IsAllowed(ctx, map[string]interface{}{
 			"role":    role,
 			"action":  action,
 			"subject": sub,