From 73e75ebfdb287e550437788cb0111888b9c5445b Mon Sep 17 00:00:00 2001
From: Bailin He <15058035+bailinhe@users.noreply.github.com>
Date: Mon, 30 Sep 2024 09:42:14 -0400
Subject: [PATCH] role-binding: remove at least one subject requirement (#291)

* remove at least one subject requirement

Signed-off-by: Bailin He <bahe@equinix.com>

* allow updates to remove all subjects

Signed-off-by: Bailin He <bahe@equinix.com>

---------

Signed-off-by: Bailin He <bahe@equinix.com>
---
 internal/query/errors.go            |  4 ----
 internal/query/relations.go         |  6 +++---
 internal/query/rolebindings.go      | 10 +---------
 internal/query/rolebindings_test.go | 22 +++++++++++-----------
 internal/query/roles_v2.go          |  2 +-
 5 files changed, 16 insertions(+), 28 deletions(-)

diff --git a/internal/query/errors.go b/internal/query/errors.go
index 07acb3bd..f43f1f2f 100644
--- a/internal/query/errors.go
+++ b/internal/query/errors.go
@@ -57,10 +57,6 @@ var (
 	// request attempts to use a resource that does not support role binding v2
 	ErrResourceDoesNotSupportRoleBindingV2 = fmt.Errorf("%w: resource does not support role binding v2", ErrInvalidArgument)
 
-	// ErrCreateRoleBindingWithNoSubjects represents an error when a role
-	// binding is created with no subjects
-	ErrCreateRoleBindingWithNoSubjects = fmt.Errorf("%w: role binding must have at least one subject", ErrInvalidArgument)
-
 	// ErrRoleBindingHasNoRelationships represents an internal error when a
 	// role binding has no relationships
 	ErrRoleBindingHasNoRelationships = errors.New("role binding has no relationships")
diff --git a/internal/query/relations.go b/internal/query/relations.go
index 58480bf0..be387a2a 100644
--- a/internal/query/relations.go
+++ b/internal/query/relations.go
@@ -362,8 +362,8 @@ func (e *engine) CreateRole(ctx context.Context, actor, res types.Resource, role
 
 // diff determines which entities needs to be added and removed.
 // If no new entity are provided it is assumed no changes are requested.
-func diff(current, incoming []string) ([]string, []string) {
-	if len(incoming) == 0 {
+func diff(current, incoming []string, allowEmptyIncoming bool) ([]string, []string) {
+	if len(incoming) == 0 && !allowEmptyIncoming {
 		return nil, nil
 	}
 
@@ -445,7 +445,7 @@ func (e *engine) UpdateRole(ctx context.Context, actor, roleResource types.Resou
 		newName = role.Name
 	}
 
-	addActions, remActions := diff(role.Actions, newActions)
+	addActions, remActions := diff(role.Actions, newActions, false)
 
 	// If no changes, return existing role with no changes.
 	if newName == role.Name && len(addActions) == 0 && len(remActions) == 0 {
diff --git a/internal/query/rolebindings.go b/internal/query/rolebindings.go
index 80b00349..7250bc41 100644
--- a/internal/query/rolebindings.go
+++ b/internal/query/rolebindings.go
@@ -103,14 +103,6 @@ func (e *engine) CreateRoleBinding(
 	)
 	defer span.End()
 
-	if len(subjects) == 0 {
-		err := ErrCreateRoleBindingWithNoSubjects
-		span.RecordError(err)
-		span.SetStatus(codes.Error, err.Error())
-
-		return types.RoleBinding{}, err
-	}
-
 	if err := e.isRoleBindable(ctx, roleResource, resource); err != nil {
 		span.RecordError(err)
 		span.SetStatus(codes.Error, err.Error())
@@ -467,7 +459,7 @@ func (e *engine) UpdateRoleBinding(ctx context.Context, actor, rb types.Resource
 		newSubjectIDs[i] = subj.SubjectResource.ID
 	}
 
-	add, remove := diff(current, incoming)
+	add, remove := diff(current, incoming, true)
 
 	// return if there are no changes
 	if (len(add) + len(remove)) == 0 {
diff --git a/internal/query/rolebindings_test.go b/internal/query/rolebindings_test.go
index d0f294b9..d9412db3 100644
--- a/internal/query/rolebindings_test.go
+++ b/internal/query/rolebindings_test.go
@@ -121,17 +121,6 @@ func TestCreateRoleBinding(t *testing.T) {
 				assert.ErrorIs(t, res.Err, ErrRoleNotFound)
 			},
 		},
-		{
-			Name: "CreateRoleBindingWithNoSubjects",
-			Input: input{
-				resource: root,
-				role:     roleRes,
-			},
-			CheckFn: func(ctx context.Context, t *testing.T, tr testingx.TestResult[types.RoleBinding]) {
-				assert.ErrorIs(t, tr.Err, ErrInvalidArgument)
-				assert.ErrorIs(t, tr.Err, ErrCreateRoleBindingWithNoSubjects)
-			},
-		},
 		{
 			Name: "CreateRoleBindingSuccess",
 			Input: input{
@@ -405,6 +394,17 @@ func TestUpdateRoleBinding(t *testing.T) {
 			},
 			Sync: true,
 		},
+		{
+			Name: "UpdateRoleBindingRemoveAllSubjects",
+			Input: input{
+				rb:   rbRes,
+				subj: []types.RoleBindingSubject{},
+			},
+			CheckFn: func(ctx context.Context, t *testing.T, res testingx.TestResult[types.RoleBinding]) {
+				assert.NoError(t, res.Err)
+				assert.Len(t, res.Success.SubjectIDs, 0)
+			},
+		},
 	}
 
 	testFn := func(ctx context.Context, in input) testingx.TestResult[types.RoleBinding] {
diff --git a/internal/query/roles_v2.go b/internal/query/roles_v2.go
index bdfa9287..5ef55924 100644
--- a/internal/query/roles_v2.go
+++ b/internal/query/roles_v2.go
@@ -260,7 +260,7 @@ func (e *engine) UpdateRoleV2(ctx context.Context, actor, roleResource types.Res
 		newName = role.Name
 	}
 
-	addActions, rmActions := diff(role.Actions, newActions)
+	addActions, rmActions := diff(role.Actions, newActions, false)
 
 	// If no changes, return existing role
 	if newName == role.Name && len(addActions) == 0 && len(rmActions) == 0 {