Repository: numaproj/numaflow
Pull request #1359 (2023-11-14 21:59:22)

chore: add unit tests for authZ


Patch:
diff --git a/server/authz/options.go b/server/authz/options.go
new file mode 100644
index 00000000..a7ea8084
--- /dev/null
+++ b/server/authz/options.go
@@ -0,0 +1,31 @@
+package authz
+
+type options struct {
+	policyMapPath      string
+	rbacPropertiesPath string
+}
+
+// Option is the interface to apply options.
+type Option func(*options)
+
+func DefaultOptions() *options {
+	return &options{
+		policyMapPath:      policyMapPath,
+		rbacPropertiesPath: rbacPropertiesPath,
+	}
+}
+
+// WithPolicyMap sets the policy map path to be used for the RBAC enforcer
+func WithPolicyMap(path string) Option {
+	return func(opts *options) {
+		opts.policyMapPath = path
+	}
+}
+
+// WithPropertyFile sets the property file path to be used for the RBAC enforcer
+func WithPropertyFile(path string) Option {
+	return func(opts *options) {
+		opts.rbacPropertiesPath = path
+	}
+
+}
diff --git a/server/authz/rbac.go b/server/authz/rbac.go
index 66e5818f..14756bcd 100644
--- a/server/authz/rbac.go
+++ b/server/authz/rbac.go
@@ -45,21 +45,39 @@ const (
 	emptyString = ""
 )
 
+// CasbinObject is the struct that implements the Authorizer interface.
+// It contains the Casbin Enforcer, the current scopes, the default policy and the config reader.
+// The config reader is used to watch for changes in the config file.
+// The Casbin Enforcer is used to enforce the authorization policy.
+// The current scopes are used to determine the user identity token to be used for authorization.
+// policyDefault is the default policy to be used when the requested resource is not present in the policy.
+// userPermCount is a cache to store the count of permissions for a user. If the user has permissions in the
+// policy, we store the count in the cache and return based on the value.
 type CasbinObject struct {
 	enforcer      *casbin.Enforcer
 	userPermCount *sync.Map
 	currentScopes []string
 	policyDefault string
 	configReader  *viper.Viper
+	opts          *options
+	rwMutex       *sync.RWMutex
 }
 
-func NewCasbinObject() (*CasbinObject, error) {
-	enforcer, err := getEnforcer()
+// NewCasbinObject returns a new CasbinObject. It initializes the Casbin Enforcer with the model and policy.
+// It also initializes the config reader to watch for changes in the config file.
+func NewCasbinObject(inputOptions ...Option) (*CasbinObject, error) {
+	// Set the default options.
+	var opts = DefaultOptions()
+	// Apply the input options.
+	for _, inputOption := range inputOptions {
+		inputOption(opts)
+	}
+	enforcer, err := getEnforcer(opts.policyMapPath)
 	if err != nil {
 		return nil, err
 	}
 	configReader := viper.New()
-	configReader.SetConfigFile(rbacPropertiesPath)
+	configReader.SetConfigFile(opts.rbacPropertiesPath)
 	err = configReader.ReadInConfig()
 	if err != nil {
 		return nil, err
@@ -75,6 +93,8 @@ func NewCasbinObject() (*CasbinObject, error) {
 		currentScopes: currentScopes,
 		policyDefault: policyDefault,
 		configReader:  configReader,
+		opts:          opts,
+		rwMutex:       &sync.RWMutex{},
 	}
 
 	// Watch for changes in the config file.
@@ -92,7 +112,8 @@ func NewCasbinObject() (*CasbinObject, error) {
 // for the given user, if not we will allocate a default policy for the user.
 func (cas *CasbinObject) Authorize(c *gin.Context, userInfo *authn.UserInfo) bool {
 	// Get the scopes to check from the policy.
-	scopedList := getSubjectFromScope(cas.currentScopes, userInfo)
+	currentScopes := cas.getCurrentScopes()
+	scopedList := getSubjectFromScope(currentScopes, userInfo)
 	// Get the resource, object and action from the request.
 	resource := extractResource(c)
 	object := extractObject(c)
@@ -108,9 +129,10 @@ func (cas *CasbinObject) Authorize(c *gin.Context, userInfo *authn.UserInfo) boo
 	}
 	// If the user does not have any policy defined, allocate a default policy for the user.
 	if !userHasPolicies {
+		defaultPolicy := cas.getDefaultPolicy()
 		logger.Debugw("No policy defined for the user, allocating default policy",
-			"DefaultPolicy", cas.policyDefault)
-		ok := enforceCheck(cas.enforcer, cas.policyDefault, resource, object, action)
+			"DefaultPolicy", defaultPolicy)
+		ok := enforceCheck(cas.enforcer, defaultPolicy, resource, object, action)
 		if ok {
 			return ok
 		}
@@ -136,12 +158,12 @@ func getSubjectFromScope(scopes []string, userInfo *authn.UserInfo) []string {
 }
 
 // getEnforcer initializes the Casbin Enforcer with the model and policy.
-func getEnforcer() (*casbin.Enforcer, error) {
+func getEnforcer(policyFilePath string) (*casbin.Enforcer, error) {
 	modelRBAC, err := model.NewModelFromString(rbacModel)
 	if err != nil {
 		return nil, err
 	}
-	a := fileadapter.NewAdapter(policyMapPath)
+	a := fileadapter.NewAdapter(policyFilePath)
 
 	// Initialize the Casbin Enforcer with the model and policies.
 	enforcer, err := casbin.NewEnforcer(modelRBAC, a)
@@ -164,12 +186,12 @@ func patternMatch(args ...interface{}) (interface{}, error) {
 	if policy == MatchAll {
 		return true, nil
 	}
-	// pattern, namespace
+	// match as pattern
 	return path.Match(policy, req)
 }
 
 // stringMatch is used to match strings from the policy, if * is provided it will match all namespaces.
-// Otherwise, we will enforce based on the namespace provided.
+// Otherwise, we will enforce based on the string provided.
 func stringMatch(args ...interface{}) (interface{}, error) {
 	req, policy, err := extractArgs(args)
 	if err != nil {
@@ -180,7 +202,7 @@ func stringMatch(args ...interface{}) (interface{}, error) {
 	if policy == MatchAll {
 		return true, nil
 	}
-	// pattern, namespace
+	// match exact string
 	return policy == req, nil
 }
 
@@ -224,7 +246,7 @@ func extractObject(c *gin.Context) string {
 	return emptyString
 }
 
-// getRbacProperty is used to read the rbacPropertiesPath file path and extract the policy provided as argument,
+// getRbacProperty is used to read the RBAC property file path and extract the policy provided as argument,
 func getRbacProperty(property string, config *viper.Viper) interface{} {
 	val := config.Get(property)
 	if val == nil {
@@ -273,13 +295,14 @@ func (cas *CasbinObject) configFileReload(e fsnotify.Event) {
 	}
 	// update the scopes
 	newScopes := getRBACScopes(cas.configReader)
-	cas.currentScopes = newScopes
+	cas.setCurrentScopes(newScopes)
 	// update the default policy
-	cas.policyDefault = getDefaultPolicy(cas.configReader)
+	policyDefault := getDefaultPolicy(cas.configReader)
+	cas.setDefaultPolicy(policyDefault)
 	// clear the userPermCount cache
 	cas.userPermCount = &sync.Map{}
 
-	logger.Infow("Auth Scopes Updated", "scopes", cas.currentScopes)
+	logger.Infow("Auth Scopes Updated", "scopes", cas.getCurrentScopes())
 }
 
 // getDefaultPolicy returns the default policy from the rbac properties file. The default policy is used when the
@@ -317,3 +340,37 @@ func (cas *CasbinObject) hasPermissionsDefined(user string) bool {
 	cas.userPermCount.Store(user, hasPerms)
 	return hasPerms
 }
+
+// setCurrentScopes sets the current scopes to the given scopes.
+// It is used to update the scopes when the config file is changed.
+// It is thread safe.
+func (cas *CasbinObject) setCurrentScopes(scopes []string) {
+	cas.rwMutex.Lock()
+	defer cas.rwMutex.Unlock()
+	cas.currentScopes = scopes
+}
+
+// getCurrentScopes returns the current scopes from the CasbinObject.
+// It is thread safe.
+func (cas *CasbinObject) getCurrentScopes() []string {
+	cas.rwMutex.RLock()
+	defer cas.rwMutex.RUnlock()
+	return cas.currentScopes
+}
+
+// setDefaultPolicy sets the default policy to the given policy.
+// It is used to update the default policy when the config file is changed.
+// It is thread safe.
+func (cas *CasbinObject) setDefaultPolicy(policy string) {
+	cas.rwMutex.Lock()
+	defer cas.rwMutex.Unlock()
+	cas.policyDefault = policy
+}
+
+// getDefaultPolicy returns the default policy from the CasbinObject.
+// It is thread safe.
+func (cas *CasbinObject) getDefaultPolicy() string {
+	cas.rwMutex.RLock()
+	defer cas.rwMutex.RUnlock()
+	return cas.policyDefault
+}


Test patch:
diff --git a/server/authz/options_test.go b/server/authz/options_test.go
new file mode 100644
index 00000000..4bec6172
--- /dev/null
+++ b/server/authz/options_test.go
@@ -0,0 +1,33 @@
+package authz
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// TestWithPolicyMap is a test implementation of the WithPolicyMap function.
+// It tests that the policy map path is set correctly.
+func TestWithPolicyMap(t *testing.T) {
+	var (
+		testPolicyMapPath = "test-policy-map-path"
+		opts              = &options{
+			policyMapPath: policyMapPath,
+		}
+	)
+	WithPolicyMap(testPolicyMapPath)(opts)
+	assert.Equal(t, testPolicyMapPath, opts.policyMapPath)
+}
+
+// TestWithPropertyFile is a test implementation of the WithPropertyFile function.
+// It tests that the property file path is set correctly.
+func TestWithPropertyFile(t *testing.T) {
+	var (
+		testPropertyFilePath = "test-property-file-path"
+		opts                 = &options{
+			rbacPropertiesPath: rbacPropertiesPath,
+		}
+	)
+	WithPropertyFile(testPropertyFilePath)(opts)
+	assert.Equal(t, testPropertyFilePath, opts.rbacPropertiesPath)
+}
diff --git a/server/authz/rbac_test.go b/server/authz/rbac_test.go
new file mode 100644
index 00000000..3a0f66a5
--- /dev/null
+++ b/server/authz/rbac_test.go
@@ -0,0 +1,306 @@
+package authz
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"testing"
+	"time"
+
+	"github.com/gin-gonic/gin"
+	"github.com/stretchr/testify/assert"
+
+	"github.com/numaproj/numaflow/server/authn"
+)
+
+const (
+	testPolicyMapPath          = "testdata/test-policy-map.csv"
+	testPropertyFilePath       = "testdata/test-rbac-conf.yaml"
+	testPropertyReloadFilePath = "testdata/test-rbac-conf-reload.yaml"
+	testUrlPath                = "/test"
+	testEmail                  = "test@test.com"
+)
+
+var (
+	groupReadOnly  = []string{"role:readonly"}
+	groupAdmin     = []string{"role:admin"}
+	groupDefault   = []string{"role:dev"}
+	groupNamespace = []string{"role:namespace"}
+)
+
+// TestCreateAuthorizer is a test implementation of the NewCasbinObject function.
+// It checks that the authorizer is created correctly and the policies, configs are loaded correctly.
+func TestCreateAuthorizer(t *testing.T) {
+	authorizer, err := NewCasbinObject(WithPolicyMap(testPolicyMapPath), WithPropertyFile(testPropertyFilePath))
+	assert.NoError(t, err)
+
+	// Test that the authorizer is not nil
+	assert.NotNil(t, authorizer)
+
+	// Check the roles loaded from the policy map
+	roles := authorizer.enforcer.GetAllRoles()
+	assert.Equal(t, 2, len(roles))
+
+	// Check the default policy, which is set to "role:readonly" in the test data
+	defaultPolicy := authorizer.getDefaultPolicy()
+	assert.Equal(t, "role:readonly", defaultPolicy)
+
+	// Check the scopes loaded from the policy map
+	scopes := authorizer.getCurrentScopes()
+	assert.Equal(t, 1, len(scopes))
+	assert.Equal(t, "groups", scopes[0])
+}
+
+// TestAuthorize is a test implementation of the Authorize functionality.
+// It tests that the user is authorized correctly for the given request.
+func TestAuthorize(t *testing.T) {
+	authorizer, err := NewCasbinObject(WithPolicyMap(testPolicyMapPath), WithPropertyFile(testPropertyFilePath))
+	assert.NoError(t, err)
+
+	// Test that the authorizer is not nil
+	assert.NotNil(t, authorizer)
+
+	// get a test gin context for GET request
+	getRequest := mockHttpRequest("GET", "")
+
+	// Test that the user is authorized for the GET request
+	readOnlyUser := mockUserInfo(testEmail, groupReadOnly)
+	isAuth := authorizer.Authorize(getRequest, readOnlyUser)
+	assert.True(t, isAuth)
+
+	// Test that the admin user is authorized for the GET request
+	adminUser := mockUserInfo(testEmail, groupAdmin)
+	isAuth = authorizer.Authorize(getRequest, adminUser)
+	assert.True(t, isAuth)
+
+	postRequest := mockHttpRequest("POST", "")
+
+	// Test that the ReadOnly user is not
+	// authorized for the POST request
+	isAuth = authorizer.Authorize(postRequest, readOnlyUser)
+	assert.False(t, isAuth)
+
+	// Test that the admin user is authorized for the POST request
+	isAuth = authorizer.Authorize(postRequest, adminUser)
+	assert.True(t, isAuth)
+}
+
+// TestDefaultPolicy is a test implementation of the DefaultPolicy functionality.
+// It tests that the default policy is set correctly when the authorizer is created.
+// Additionally, it tests that the default policy is applied correctly when a user is not found in the policy map.
+// The default policy is set to "role:readonly" in the test data.
+func TestDefaultPolicy(t *testing.T) {
+	authorizer, err := NewCasbinObject(WithPolicyMap(testPolicyMapPath), WithPropertyFile(testPropertyFilePath))
+	assert.NoError(t, err)
+
+	// Test that the authorizer is not nil
+	assert.NotNil(t, authorizer)
+
+	// get a test gin context for GET request
+	// The current default policy is "role:readonly"
+	// The default policy allows only GET requests
+	// So, the GET request should be authorized
+	getRequest := mockHttpRequest("GET", "")
+	userInfo := mockUserInfo(testEmail, groupDefault)
+	isAuth := authorizer.Authorize(getRequest, userInfo)
+	assert.True(t, isAuth)
+
+	// get a test gin context for POST request
+	// The default policy allows only GET requests
+	// So, the POST request should not be authorized
+	postRequest := mockHttpRequest("POST", "")
+	isAuth = authorizer.Authorize(postRequest, userInfo)
+	assert.False(t, isAuth)
+}
+
+// TestScopes is a test implementation of the Scopes functionality.
+// It tests that the scopes are set correctly when the authorizer is created.
+// Additionally, it tests that the required scopes are tested for the user.
+func TestScopes(t *testing.T) {
+	authorizer, err := NewCasbinObject(WithPolicyMap(testPolicyMapPath), WithPropertyFile(testPropertyFilePath))
+	assert.NoError(t, err)
+
+	// Test that the authorizer is not nil
+	assert.NotNil(t, authorizer)
+
+	// Get a mock user info for read only group
+	userInfo := mockUserInfo(testEmail, groupReadOnly)
+
+	// get a test gin context for GET request
+	// The default policy allows only GET requests
+	// So, the GET request should be authorized
+	getRequest := mockHttpRequest("GET", "")
+	isAuth := authorizer.Authorize(getRequest, userInfo)
+	assert.True(t, isAuth)
+
+	// Change the scopes to "email"
+	authorizer.setCurrentScopes([]string{"email"})
+
+	// Check if the scopes are updated correctly
+	scopes := authorizer.getCurrentScopes()
+	assert.Equal(t, 1, len(scopes))
+	assert.Equal(t, "email", scopes[0])
+
+	// GET requests are for the test user as part of the read only group
+	isAuth = authorizer.Authorize(getRequest, userInfo)
+	assert.True(t, isAuth)
+
+	// POST requests are allowed for the test user email
+	postRequest := mockHttpRequest("POST", "")
+	isAuth = authorizer.Authorize(postRequest, userInfo)
+	assert.True(t, isAuth)
+
+	// PATCH requests are not allowed for the test user email
+	patchRequest := mockHttpRequest("PATCH", "")
+	isAuth = authorizer.Authorize(patchRequest, userInfo)
+	assert.False(t, isAuth)
+}
+
+// TestNamespaces is a test implementation of the Namespaces based access.
+// It tests that a user can access a namespace that is in the policy map.
+func TestNamespaces(t *testing.T) {
+	authorizer, err := NewCasbinObject(WithPolicyMap(testPolicyMapPath), WithPropertyFile(testPropertyFilePath))
+	assert.NoError(t, err)
+
+	// Test that the authorizer is not nil
+	assert.NotNil(t, authorizer)
+
+	userInfo := mockUserInfo(testEmail, groupNamespace)
+
+	// Request for a namespace that is not authorized for the user
+	getRequest := mockHttpRequest("GET", "")
+	isAuth := authorizer.Authorize(getRequest, userInfo)
+	assert.False(t, isAuth)
+
+	// Request for a namespace that is authorized for the user
+	getRequest = mockHttpRequest("GET", "test_ns")
+	isAuth = authorizer.Authorize(getRequest, userInfo)
+	assert.True(t, isAuth)
+
+}
+
+// TestConfigFileReload is a test implementation of the ConfigFileReload functionality.
+// It tests that the RBAC properties are reloaded correctly when the config file is changed.
+func TestConfigFileReload(t *testing.T) {
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+	authorizer, err := NewCasbinObject(WithPolicyMap(testPolicyMapPath), WithPropertyFile(testPropertyReloadFilePath))
+	assert.NoError(t, err)
+
+	// Test that the authorizer is not nil
+	assert.NotNil(t, authorizer)
+
+	//read the contents of source file
+	dataOrig, _ := os.ReadFile(testPropertyReloadFilePath)
+
+	// Load the RBAC properties
+	scopes := authorizer.getCurrentScopes()
+	for scopes[0] != "email" {
+		fmt.Println("Waiting for RBAC config to be reloaded")
+		select {
+		case <-ctx.Done():
+			break
+		default:
+			time.Sleep(10 * time.Millisecond)
+			scopes = authorizer.getCurrentScopes()
+		}
+	}
+	assert.Equal(t, "email", scopes[0])
+
+	//read the contents of source file
+	data, err := os.ReadFile(testPropertyFilePath)
+	if err != nil {
+		fmt.Println("Error reading file:", err)
+		return
+	}
+
+	// write the content to destination file, this will overwrite the content of policy file
+	// and the RBAC config should be reloaded
+	err = os.WriteFile(testPropertyReloadFilePath, data, 0644)
+	if err != nil {
+		fmt.Println("Error writing file:", err)
+		return
+	}
+	fmt.Println("File copied successfully")
+
+	// Check if the RBAC properties are reloaded correctly
+	scopes = authorizer.getCurrentScopes()
+	for scopes[0] != "groups" {
+		fmt.Println("Waiting for RBAC config to be reloaded")
+		select {
+		case <-ctx.Done():
+			break
+		default:
+			time.Sleep(10 * time.Millisecond)
+			scopes = authorizer.getCurrentScopes()
+		}
+	}
+	assert.Equal(t, "groups", scopes[0])
+
+	// write the original content back to the file, this should trigger the RBAC config reload
+	err = os.WriteFile(testPropertyReloadFilePath, dataOrig, 0644)
+	if err != nil {
+		fmt.Println("Error writing file:", err)
+		return
+	}
+	fmt.Println("File copied successfully")
+
+	// Check if the RBAC properties are reloaded correctly
+	scopes = authorizer.getCurrentScopes()
+	for scopes[0] != "email" {
+		fmt.Println("Waiting for RBAC config to be reloaded")
+		select {
+		case <-ctx.Done():
+			break
+		default:
+			time.Sleep(10 * time.Millisecond)
+			scopes = authorizer.getCurrentScopes()
+		}
+	}
+	assert.Equal(t, "email", scopes[0])
+}
+
+// createTestGinContext is a helper function to create a test gin context.
+func createTestGinContext() *gin.Context {
+	// Create a new HTTP response recorder
+	w := httptest.NewRecorder()
+
+	// Create a new Gin context with the request and response recorder
+	c, _ := gin.CreateTestContext(w)
+	return c
+}
+
+// mockHttpRequest is a helper function to create a mock HTTP request.
+// It takes the HTTP method and namespace as input.
+// If the namespace is empty, it sets the namespace to "default".
+// It returns a struct of type gin.Context
+func mockHttpRequest(method string, namespace string) *gin.Context {
+	req, _ := http.NewRequest(method, testUrlPath, nil)
+	c := createTestGinContext()
+	c.Request = req
+	if namespace != "" {
+		c.Params = append(c.Params, gin.Param{Key: "namespace", Value: namespace})
+	} else {
+		// Set the namespace to "default"
+		c.Params = append(c.Params, gin.Param{Key: "namespace", Value: "default"})
+	}
+	return c
+}
+
+// mockUserInfo is a helper function to mock the user info. It returns a struct of type UserInfo
+func mockUserInfo(email string, groups []string) *authn.UserInfo {
+	idClaims := mockIdClaims(groups, email)
+	return &authn.UserInfo{
+		IDTokenClaims: idClaims,
+	}
+}
+
+// mockIdClaims is a helper function to mock the IDTokenClaims. It returns a struct of type IDTokenClaims
+func mockIdClaims(groups []string, email string) *authn.IDTokenClaims {
+	return &authn.IDTokenClaims{
+		Email:  email,
+		Groups: groups,
+	}
+}
diff --git a/server/authz/testdata/test-policy-map.csv b/server/authz/testdata/test-policy-map.csv
new file mode 100644
index 00000000..66ca3d02
--- /dev/null
+++ b/server/authz/testdata/test-policy-map.csv
@@ -0,0 +1,9 @@
+# Policies go here
+p, role:admin, *, *, *
+p, role:readonly, *, *, GET
+p, test@test.com, *, *, POST
+p, role:namespace, test_ns, *, *
+# Groups go here
+g, admin, role:admin
+g, readonly, role:readonly
+g, test@test.com, role:readonly
\ No newline at end of file
diff --git a/server/authz/testdata/test-rbac-conf-reload.yaml b/server/authz/testdata/test-rbac-conf-reload.yaml
new file mode 100644
index 00000000..1910a3a1
--- /dev/null
+++ b/server/authz/testdata/test-rbac-conf-reload.yaml
@@ -0,0 +1,3 @@
+policy:
+  default: role:readonly
+  scopes: email
diff --git a/server/authz/testdata/test-rbac-conf.yaml b/server/authz/testdata/test-rbac-conf.yaml
new file mode 100644
index 00000000..6baef334
--- /dev/null
+++ b/server/authz/testdata/test-rbac-conf.yaml
@@ -0,0 +1,3 @@
+policy:
+  default: role:readonly
+  scopes: groups