Fix wrong pagination when multiple unsetting resource accesses (#1222)

* Fix wrong pagination Signed-off-by: Babak K. Shandiz <[email protected]> * Add `pageSize` parameter to `unsetMultipleResourceAccesses` Signed-off-by: Babak K. Shandiz <[email protected]> * Add test for `unsetMultipleResourceAccesses` Signed-off-by: Babak K. Shandiz <[email protected]> * Apply various page sizes Signed-off-by: Babak K. Shandiz <[email protected]> --------- Signed-off-by: Babak K. Shandiz <[email protected]>
canonical · May 30, 2024 · bba595e · bba595e
1 parent 97e203f
commit bba595e
Show file tree

Hide file tree

Showing 3 changed files with 101 additions and 8 deletions.
diff --git a/internal/openfga/export_test.go b/internal/openfga/export_test.go
@@ -2,7 +2,17 @@
 
 package openfga
 
-import "context"
+import (
+	"context"
+
+	ofganames "github.com/canonical/jimm/internal/openfga/names"
+)
+
+// This is just for exporting the private function for testing. Since the private
+// function is a generic one, we cannot use a simple file-scoped "var" statement.
+func UnsetMultipleResourceAccesses[T ofganames.ResourceTagger](ctx context.Context, user *User, resource T, relations []Relation, pageSize int32) error {
+	return unsetMultipleResourceAccesses(ctx, user, resource, relations, pageSize)
+}
 
 func (o *OFGAClient) RemoveTuples(ctx context.Context, tuple Tuple) error {
 	return o.removeTuples(ctx, tuple)

diff --git a/internal/openfga/user.go b/internal/openfga/user.go
@@ -206,7 +206,7 @@ func (u *User) SetModelAccess(ctx context.Context, resource names.ModelTag, rela
 // UnsetModelAccess removes direct relations between the user and the model.
 // Note that the action is idempotent (i.e., does not return error if the relation does not exist).
 func (u *User) UnsetModelAccess(ctx context.Context, resource names.ModelTag, relations ...Relation) error {
-	return unsetMultipleResourceAccesses(ctx, u, resource, relations)
+	return unsetMultipleResourceAccesses(ctx, u, resource, relations, 0)
 }
 
 // SetControllerAccess adds a direct relation between the user and the controller.
@@ -230,7 +230,7 @@ func (u *User) SetCloudAccess(ctx context.Context, resource names.CloudTag, rela
 // UnsetCloudAccess removes direct relations between the user and the cloud.
 // Note that the action is idempotent (i.e., does not return error if the relation does not exist).
 func (u *User) UnsetCloudAccess(ctx context.Context, resource names.CloudTag, relations ...Relation) error {
-	return unsetMultipleResourceAccesses(ctx, u, resource, relations)
+	return unsetMultipleResourceAccesses(ctx, u, resource, relations, 0)
 }
 
 // SetApplicationOfferAccess adds a direct relation between the user and the application offer.
@@ -364,9 +364,14 @@ func setResourceAccess[T ofganames.ResourceTagger](ctx context.Context, user *Us
 	return nil
 }
 
-// unsetMultipleResourceAccesses deletes relations that correspond to the requested resource access, atomically.
-// Note that the action is idempotent (i.e., does not return error if any of the relations does not exist).
-func unsetMultipleResourceAccesses[T ofganames.ResourceTagger](ctx context.Context, user *User, resource T, relations []Relation) error {
+// unsetMultipleResourceAccesses deletes relations that correspond to the
+// requested resource access, atomically. The pageSize argument determines the
+// read requests chunk size, and can be set to zero to opt to OpenFGA client
+// defaults.
+//
+// Note that the action is idempotent (i.e., does not return error if any of the
+// relations does not exist).
+func unsetMultipleResourceAccesses[T ofganames.ResourceTagger](ctx context.Context, user *User, resource T, relations []Relation, pageSize int32) error {
 	tupleObject := ofganames.ConvertTag(user.ResourceTag())
 	tupleTarget := ofganames.ConvertTag(resource)
 
@@ -376,7 +381,7 @@ func unsetMultipleResourceAccesses[T ofganames.ResourceTagger](ctx context.Conte
 		timestampedTuples, continuationToken, err := user.client.cofgaClient.FindMatchingTuples(ctx, Tuple{
 			Object: tupleObject,
 			Target: tupleTarget,
-		}, 0, lastContinuationToken)
+		}, pageSize, lastContinuationToken)
 
 		if err != nil {
 			return errors.E(err, "failed to retrieve existing relations")
@@ -386,7 +391,7 @@ func unsetMultipleResourceAccesses[T ofganames.ResourceTagger](ctx context.Conte
 			existingRelations[timestampedTuple.Tuple.Relation] = nil
 		}
 
-		if continuationToken == lastContinuationToken {
+		if continuationToken == "" {
 			break
 		}
 		lastContinuationToken = continuationToken

diff --git a/internal/openfga/user_test.go b/internal/openfga/user_test.go
@@ -576,3 +576,81 @@ func (s *userTestSuite) TestListApplicationOffers(c *gc.C) {
 	sort.Strings(offerUUIDs)
 	c.Assert(offerUUIDs, gc.DeepEquals, wantUUIDs)
 }
+
+func (s *userTestSuite) TestUnsetMultipleResourceAccesses(c *gc.C) {
+	ctx := context.Background()
+
+	tests := []struct {
+		name     string
+		pageSize int32
+	}{{
+		name:     "pageSize: 0 (OpenFGA default)",
+		pageSize: 0,
+	}, {
+		name:     "pageSize: 100 (OpenFGA max)",
+		pageSize: 100,
+	}, {
+		name:     "pageSize: 1",
+		pageSize: 1,
+	}, {
+		name:     "pageSize: 2",
+		pageSize: 2,
+	}, {
+		name:     "pageSize: 3",
+		pageSize: 3,
+	}, {
+		name:     "pageSize: 4",
+		pageSize: 4,
+	}}
+
+	for _, tt := range tests {
+		modelUUID, err := uuid.NewRandom()
+		c.Assert(err, gc.IsNil)
+		model := names.NewModelTag(modelUUID.String())
+
+		adam := names.NewUserTag("adam")
+
+		adamIdentity, err := dbmodel.NewIdentity("adam")
+		c.Assert(err, gc.IsNil)
+
+		adamUser := openfga.NewUser(adamIdentity, s.ofgaClient)
+
+		// Note that the total number of tuples in OpenFGA actually has no
+		// effect here, because the `unsetMultipleResourceAccesses` function
+		// queries for tuples that have a specific object and target. So, the
+		// returned tuples are just a few. That's why we've used user-to-model
+		// tuples in this test because they have the highest number of possible
+		// relations (i.e., reader, writer, and administrator).
+		tuples := []openfga.Tuple{{
+			Object:   ofganames.ConvertTag(adam),
+			Relation: ofganames.ReaderRelation,
+			Target:   ofganames.ConvertTag(model),
+		}, {
+			Object:   ofganames.ConvertTag(adam),
+			Relation: ofganames.WriterRelation,
+			Target:   ofganames.ConvertTag(model),
+		}, {
+			Object:   ofganames.ConvertTag(adam),
+			Relation: ofganames.AdministratorRelation,
+			Target:   ofganames.ConvertTag(model),
+		}}
+
+		err = s.ofgaClient.AddRelation(ctx, tuples...)
+		c.Assert(err, gc.IsNil)
+
+		err = openfga.UnsetMultipleResourceAccesses(
+			ctx, adamUser, model,
+			[]openfga.Relation{
+				ofganames.ReaderRelation,
+				ofganames.WriterRelation,
+				ofganames.AdministratorRelation,
+			},
+			tt.pageSize,
+		)
+		c.Assert(err, gc.IsNil)
+
+		retrieved, _, err := s.cofgaClient.FindMatchingTuples(ctx, openfga.Tuple{Target: ofganames.ConvertTag(model)}, 0, "")
+		c.Assert(err, gc.IsNil)
+		c.Assert(retrieved, gc.HasLen, 0)
+	}
+}