bugfix: existing role permissions should not be removed during update (…

…#90)

Co-authored-by: Khor Shu Heng <khor.heng@gojek.com>

api/pkg/authz/enforcer/enforcer.go

@@ -240,16 +240,6 @@ func (e *enforcer) UpdateAuthorization(ctx context.Context, updateRequest Author
 		return err
 	}
 	patches := make([]ory.RelationshipPatch, 0)
-	existingRolePermissions.Range(func(key, value interface{}) bool {
-		role := key.(string)
-		permissions := value.([]string)
-		for _, permission := range permissions {
-			if !slices.Contains(updateRequest.RolePermissions[role], permission) {
-				patches = append(patches, newRolePermissionPatch("delete", permission, role))
-			}
-		}
-		return true
-	})
 
 	for role, permissions := range updateRequest.RolePermissions {
 		for _, permission := range permissions {
@@ -291,7 +281,7 @@ func (e *enforcer) isCacheEnabled() bool {
 }
 
 // NewAuthorizationUpdateRequest create a new AuthorizationUpdateRequest. Multiple operations can be chained together
-// using the SetRolePermissions and SetRoleMembers methods. No changes will be made until the AuthorizationUpdateRequest
+// using the AddRolePermissions and SetRoleMembers methods. No changes will be made until the AuthorizationUpdateRequest
 // object is passed to the Enforcer, in which all the previously chained operations will be executed in batch.
 func NewAuthorizationUpdateRequest() AuthorizationUpdateRequest {
 	return AuthorizationUpdateRequest{
@@ -305,8 +295,8 @@ type AuthorizationUpdateRequest struct {
 	RoleMembers     map[string][]string
 }
 
-// SetRolePermissions set the permissions for a role. If the role already has permissions, they will be replaced.
-func (a AuthorizationUpdateRequest) SetRolePermissions(role string,
+// AddRolePermissions add permissions to a role, without duplication. Existing permissions will still be in place.
+func (a AuthorizationUpdateRequest) AddRolePermissions(role string,
 	permissions []string) AuthorizationUpdateRequest {
 	a.RolePermissions[role] = permissions
 	return a

api/pkg/authz/enforcer/enforcer_test.go

@@ -6,6 +6,8 @@ import (
 	"sort"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	ory "github.com/ory/keto-client-go"
 	"github.com/stretchr/testify/require"
 )
@@ -74,7 +76,7 @@ func TestEnforcer_HasPermission(t *testing.T) {
 	clearRelations(readClient, writeClient)
 
 	newRoleAndPermissionsRequest := NewAuthorizationUpdateRequest()
-	newRoleAndPermissionsRequest.SetRolePermissions("page.1.admin", []string{"page.1.get", "page.1.put"})
+	newRoleAndPermissionsRequest.AddRolePermissions("page.1.admin", []string{"page.1.get", "page.1.put"})
 	newRoleAndPermissionsRequest.SetRoleMembers("page.1.admin", []string{"user-1@example.com"})
 	err = ketoEnforcer.UpdateAuthorization(context.Background(), newRoleAndPermissionsRequest)
 	require.NoError(t, err)
@@ -118,7 +120,7 @@ func TestEnforcer_HasPermission(t *testing.T) {
 		})
 	}
 	updateRoleAndPermissionsRequest := NewAuthorizationUpdateRequest()
-	updateRoleAndPermissionsRequest.SetRolePermissions("page.1.admin", []string{"page.1.get", "page.1.delete"})
+	updateRoleAndPermissionsRequest.AddRolePermissions("page.1.admin", []string{"page.1.get", "page.1.delete"})
 	updateRoleAndPermissionsRequest.SetRoleMembers("page.1.admin", []string{"admin-1@example.com"})
 	err = ketoEnforcer.UpdateAuthorization(context.Background(), updateRoleAndPermissionsRequest)
 	testsAfterUpdate := []struct {
@@ -140,10 +142,10 @@ func TestEnforcer_HasPermission(t *testing.T) {
 			true,
 		},
 		{
-			"reject after update: admin-1 request update page.1",
+			"allow after update: admin-1 request update page.1",
 			"page.1.put",
 			"admin-1@example.com",
-			false,
+			true,
 		},
 	}
 	for _, tt := range testsAfterUpdate {
@@ -208,7 +210,7 @@ func TestEnforcer_GetRolePermissions(t *testing.T) {
 	writeClient := newKetoClient(ketoRemoteWrite)
 	clearRelations(readClient, writeClient)
 	updateRequest := NewAuthorizationUpdateRequest()
-	updateRequest.SetRolePermissions("pages.1.reader", []string{"pages.1.get", "pages.1.post"})
+	updateRequest.AddRolePermissions("pages.1.reader", []string{"pages.1.get", "pages.1.post"})
 	err = ketoEnforcer.UpdateAuthorization(context.Background(), updateRequest)
 	require.NoError(t, err)
 	tests := []struct {
@@ -248,8 +250,8 @@ func TestEnforcer_GetUserPermissions(t *testing.T) {
 	writeClient := newKetoClient(ketoRemoteWrite)
 	clearRelations(readClient, writeClient)
 	updateRequest := NewAuthorizationUpdateRequest()
-	updateRequest.SetRolePermissions("pages.1.reader", []string{"pages.1.get"})
-	updateRequest.SetRolePermissions("pages.1.admin", []string{"pages.1.get", "pages.1.post"})
+	updateRequest.AddRolePermissions("pages.1.reader", []string{"pages.1.get"})
+	updateRequest.AddRolePermissions("pages.1.admin", []string{"pages.1.get", "pages.1.post"})
 	updateRequest.SetRoleMembers("pages.1.reader", []string{"user-1@example.com"})
 	updateRequest.SetRoleMembers("pages.1.admin", []string{"user-1@example.com"})
 	err = ketoEnforcer.UpdateAuthorization(context.Background(), updateRequest)
@@ -284,14 +286,45 @@ func TestEnforcer_GetUserPermissions(t *testing.T) {
 	}
 }
 
+func TestEnforcer_UpdateAuthorization(t *testing.T) {
+	ketoEnforcer, err := NewEnforcerBuilder().Build()
+	require.NoError(t, err)
+	readClient := newKetoClient(ketoRemoteRead)
+	writeClient := newKetoClient(ketoRemoteWrite)
+	clearRelations(readClient, writeClient)
+	updateRequest := NewAuthorizationUpdateRequest()
+	updateRequest.AddRolePermissions("pages.readers", []string{"pages.1.get", "pages.1.post"})
+	err = ketoEnforcer.UpdateAuthorization(context.Background(), updateRequest)
+	require.NoError(t, err)
+	res, err := ketoEnforcer.GetRolePermissions(context.Background(), "pages.readers")
+	require.NoError(t, err)
+	sort.Strings(res)
+	assert.Equal(t, []string{"pages.1.get", "pages.1.post"}, res)
+	updateRequest = NewAuthorizationUpdateRequest()
+	updateRequest.AddRolePermissions("pages.readers", []string{"pages.2.get", "pages.2.post"})
+	err = ketoEnforcer.UpdateAuthorization(context.Background(), updateRequest)
+	require.NoError(t, err)
+	res, err = ketoEnforcer.GetRolePermissions(context.Background(), "pages.readers")
+	require.NoError(t, err)
+	sort.Strings(res)
+	assert.Equal(t, []string{"pages.1.get", "pages.1.post", "pages.2.get", "pages.2.post"}, res)
+	updateRequest.AddRolePermissions("pages.readers", []string{"pages.2.get", "pages.2.post"})
+	err = ketoEnforcer.UpdateAuthorization(context.Background(), updateRequest)
+	require.NoError(t, err)
+	res, err = ketoEnforcer.GetRolePermissions(context.Background(), "pages.readers")
+	require.NoError(t, err)
+	sort.Strings(res)
+	assert.Equal(t, []string{"pages.1.get", "pages.1.post", "pages.2.get", "pages.2.post"}, res)
+}
+
 func TestEnforcer_GetRoleMembers(t *testing.T) {
 	ketoEnforcer, err := NewEnforcerBuilder().Build()
 	require.NoError(t, err)
 	readClient := newKetoClient(ketoRemoteRead)
 	writeClient := newKetoClient(ketoRemoteWrite)
 	clearRelations(readClient, writeClient)
 	updateRequest := NewAuthorizationUpdateRequest()
-	updateRequest.SetRolePermissions("pages.1.reader", []string{"pages.1.get", "pages.1.post"})
+	updateRequest.AddRolePermissions("pages.1.reader", []string{"pages.1.get", "pages.1.post"})
 	err = ketoEnforcer.UpdateAuthorization(context.Background(), updateRequest)
 	require.NoError(t, err)
 	tests := []struct {

api/service/projects_service.go

@@ -144,7 +144,7 @@ func (service *projectsService) updateAuthorizationPolicy(ctx context.Context, p
 		return err
 	}
 	for _, role := range rolesWithReadOnlyAccess {
-		updateRequest.SetRolePermissions(role, readPermissions(project))
+		updateRequest.AddRolePermissions(role, readPermissions(project))
 	}
 	projectAdminRole, err := enforcer.ParseProjectRole(enforcer.MLPProjectAdminRole, project)
 	if err != nil {
@@ -164,7 +164,7 @@ func (service *projectsService) updateAuthorizationPolicy(ctx context.Context, p
 		return err
 	}
 	for _, role := range rolesWithAdminAccess {
-		updateRequest.SetRolePermissions(role, adminPermissions(project))
+		updateRequest.AddRolePermissions(role, adminPermissions(project))
 	}
 	projectReaderRole, err := enforcer.ParseProjectRole(enforcer.MLPProjectReaderRole, project)
 	if err != nil {

go.mod

@@ -32,6 +32,7 @@ require (
 	github.com/prometheus/client_model v0.2.0
 	github.com/rs/cors v1.7.0
 	github.com/spf13/cobra v1.7.0
+	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.8.1
 	github.com/uber/jaeger-client-go v2.16.0+incompatible
 	go.uber.org/zap v1.17.0
@@ -105,7 +106,6 @@ require (
 	github.com/ryanuber/go-glob v1.0.0 // indirect
 	github.com/sanity-io/litter v1.5.5 // indirect
 	github.com/sergi/go-diff v1.0.0 // indirect
-	github.com/spf13/pflag v1.0.5 // indirect
 	github.com/stretchr/objx v0.5.0 // indirect
 	github.com/uber-go/atomic v1.4.0 // indirect
 	github.com/uber/jaeger-lib v2.0.0+incompatible // indirect