change interpretation of group parameter for PatchIdentityGroups (#1423)

The group parameter passed to the PatchIdentityGroups method is a group ID not a group tag (not group-ID and instead just ID). This PR fixes that misinterpretation. Co-authored-by: Ales Stimec <[email protected]>
canonical · Nov 11, 2024 · 4e82139 · 4e82139
1 parent 93a8cf4
commit 4e82139
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 17 deletions.
diff --git a/internal/jimmhttp/rebac_admin/identities.go b/internal/jimmhttp/rebac_admin/identities.go
@@ -19,6 +19,7 @@ import (
 	"github.com/canonical/jimm/v3/internal/openfga"
 	ofganames "github.com/canonical/jimm/v3/internal/openfga/names"
 	apiparams "github.com/canonical/jimm/v3/pkg/api/params"
+	jimmnames "github.com/canonical/jimm/v3/pkg/names"
 )
 
 type identitiesService struct {
@@ -158,10 +159,13 @@ func (s *identitiesService) PatchIdentityGroups(ctx context.Context, identityId
 	additions := make([]apiparams.RelationshipTuple, 0)
 	deletions := make([]apiparams.RelationshipTuple, 0)
 	for _, p := range groupPatches {
+		if !jimmnames.IsValidGroupId(p.Group) {
+			return false, v1.NewValidationError(fmt.Sprintf("ID %s is not a valid group ID", p.Group))
+		}
 		t := apiparams.RelationshipTuple{
 			Object:       objUser.ResourceTag().String(),
 			Relation:     ofganames.MemberRelation.String(),
-			TargetObject: p.Group,
+			TargetObject: jimmnames.NewGroupTag(p.Group).String(),
 		}
 		if p.Op == "add" {
 			additions = append(additions, t)

diff --git a/internal/jimmhttp/rebac_admin/identities_integration_test.go b/internal/jimmhttp/rebac_admin/identities_integration_test.go
@@ -31,11 +31,11 @@ func (s *identitiesSuite) TestIdentityPatchGroups(c *gc.C) {
 	identitySvc := rebac_admin.NewidentitiesService(s.JIMM)
 	groupName := "group-test1"
 	username := s.AdminUser.Name
-	groupTag := s.AddGroup(c, groupName)
+	group := s.AddGroup(c, groupName)
 
 	// test add identity group
 	changed, err := identitySvc.PatchIdentityGroups(ctx, username, []resources.IdentityGroupsPatchItem{{
-		Group: groupTag.String(),
+		Group: group.UUID,
 		Op:    resources.IdentityGroupsPatchItemOpAdd,
 	}})
 	c.Assert(err, gc.IsNil)
@@ -47,23 +47,23 @@ func (s *identitiesSuite) TestIdentityPatchGroups(c *gc.C) {
 	tuples, _, err := s.JIMM.ListRelationshipTuples(ctx, s.AdminUser, params.RelationshipTuple{
 		Object:       objUser.ResourceTag().String(),
 		Relation:     ofganames.MemberRelation.String(),
-		TargetObject: groupTag.String(),
+		TargetObject: group.ResourceTag().String(),
 	}, 10, "")
 	c.Assert(err, gc.IsNil)
 	c.Assert(len(tuples), gc.Equals, 1)
-	c.Assert(groupTag.Id(), gc.Equals, tuples[0].Target.ID)
+	c.Assert(group.UUID, gc.Equals, tuples[0].Target.ID)
 
 	// test user remove from group
 	changed, err = identitySvc.PatchIdentityGroups(ctx, username, []resources.IdentityGroupsPatchItem{{
-		Group: groupTag.String(),
+		Group: group.UUID,
 		Op:    resources.IdentityGroupsPatchItemOpRemove,
 	}})
 	c.Assert(err, gc.IsNil)
 	c.Assert(changed, gc.Equals, true)
 	tuples, _, err = s.JIMM.ListRelationshipTuples(ctx, s.AdminUser, params.RelationshipTuple{
 		Object:       objUser.ResourceTag().String(),
 		Relation:     ofganames.MemberRelation.String(),
-		TargetObject: groupTag.String(),
+		TargetObject: group.ResourceTag().String(),
 	}, 10, "")
 	c.Assert(err, gc.IsNil)
 	c.Assert(len(tuples), gc.Equals, 0)
@@ -80,10 +80,10 @@ func (s *identitiesSuite) TestIdentityGetGroups(c *gc.C) {
 	groupTags := make([]jimmnames.GroupTag, groupsSize)
 	for i := range groupsSize {
 		groupName := fmt.Sprintf("group-test%d", i)
-		groupTag := s.AddGroup(c, groupName)
-		groupTags[i] = groupTag
+		group := s.AddGroup(c, groupName)
+		groupTags[i] = group.ResourceTag()
 		groupsToAdd[i] = resources.IdentityGroupsPatchItem{
-			Group: groupTag.String(),
+			Group: group.UUID,
 			Op:    resources.IdentityGroupsPatchItemOpAdd,
 		}
 
@@ -117,13 +117,13 @@ func (s *identitiesSuite) TestIdentityEntitlements(c *gc.C) {
 	// initialization
 	ctx := context.Background()
 	identitySvc := rebac_admin.NewidentitiesService(s.JIMM)
-	groupTag := s.AddGroup(c, "test-group")
+	group := s.AddGroup(c, "test-group")
 	user := names.NewUserTag("[email protected]")
 	s.AddUser(c, user.Id())
 	err := s.JIMM.OpenFGAClient.AddRelation(ctx, openfga.Tuple{
 		Object:   ofganames.ConvertTag(user),
 		Relation: ofganames.MemberRelation,
-		Target:   ofganames.ConvertTag(groupTag),
+		Target:   ofganames.ConvertTag(group.ResourceTag()),
 	})
 	c.Assert(err, gc.IsNil)
 	tuple := openfga.Tuple{

diff --git a/internal/jimmhttp/rebac_admin/identities_test.go b/internal/jimmhttp/rebac_admin/identities_test.go
@@ -11,6 +11,7 @@ import (
 	rebac_handlers "github.com/canonical/rebac-admin-ui-handlers/v1"
 	"github.com/canonical/rebac-admin-ui-handlers/v1/resources"
 	qt "github.com/frankban/quicktest"
+	"github.com/google/uuid"
 
 	"github.com/canonical/jimm/v3/internal/common/pagination"
 	"github.com/canonical/jimm/v3/internal/common/utils"
@@ -210,9 +211,11 @@ func TestPatchIdentityGroups(t *testing.T) {
 	c.Assert(err, qt.ErrorMatches, ".* not found")
 
 	username := "[email protected]"
+	group1ID := uuid.New()
+	group2ID := uuid.New()
 	operations := []resources.IdentityGroupsPatchItem{
-		{Group: "test-group1", Op: resources.IdentityGroupsPatchItemOpAdd},
-		{Group: "test-group2", Op: resources.IdentityGroupsPatchItemOpRemove},
+		{Group: group1ID.String(), Op: resources.IdentityGroupsPatchItemOpAdd},
+		{Group: group2ID.String(), Op: resources.IdentityGroupsPatchItemOpRemove},
 	}
 	res, err := idSvc.PatchIdentityGroups(ctx, username, operations)
 	c.Assert(err, qt.IsNil)
@@ -221,4 +224,10 @@ func TestPatchIdentityGroups(t *testing.T) {
 	patchTuplesErr = errors.New("foo")
 	_, err = idSvc.PatchIdentityGroups(ctx, username, operations)
 	c.Assert(err, qt.ErrorMatches, ".*foo")
+
+	invalidGroupName := []resources.IdentityGroupsPatchItem{
+		{Group: "test-group1", Op: resources.IdentityGroupsPatchItemOpAdd},
+	}
+	_, err = idSvc.PatchIdentityGroups(ctx, "[email protected]", invalidGroupName)
+	c.Assert(err, qt.ErrorMatches, "Bad Request: ID test-group1 is not a valid group ID")
 }
diff --git a/internal/testutils/jimmtest/suite.go b/internal/testutils/jimmtest/suite.go
@@ -29,7 +29,6 @@ import (
 	"github.com/canonical/jimm/v3/internal/openfga"
 	ofganames "github.com/canonical/jimm/v3/internal/openfga/names"
 	"github.com/canonical/jimm/v3/internal/pubsub"
-	jimmnames "github.com/canonical/jimm/v3/pkg/names"
 )
 
 // ControllerUUID is the UUID of the JIMM controller used in tests.
@@ -267,11 +266,11 @@ func (s *JIMMSuite) AddModel(c *gc.C, owner names.UserTag, name string, cloud na
 	return names.NewModelTag(mi.UUID)
 }
 
-func (s *JIMMSuite) AddGroup(c *gc.C, groupName string) jimmnames.GroupTag {
+func (s *JIMMSuite) AddGroup(c *gc.C, groupName string) dbmodel.GroupEntry {
 	ctx := context.Background()
 	group, err := s.JIMM.AddGroup(ctx, s.AdminUser, groupName)
 	c.Assert(err, gc.Equals, nil)
-	return group.ResourceTag()
+	return *group
 }
 
 // EnableDeviceFlow allows a test to use the device flow.