diff --git a/internal/jimmhttp/rebac_admin/identities.go b/internal/jimmhttp/rebac_admin/identities.go index b21373745..3bf0f70f0 100644 --- a/internal/jimmhttp/rebac_admin/identities.go +++ b/internal/jimmhttp/rebac_admin/identities.go @@ -121,17 +121,27 @@ func (s *identitiesService) GetIdentityGroups(ctx context.Context, identityId st Relation: ofganames.MemberRelation.String(), TargetObject: openfga.GroupType.String(), }, int32(filter.Limit()), filter.Token()) // #nosec G115 accept integer conversion - if err != nil { return nil, err } - groups := make([]resources.Group, len(tuples)) - for i, t := range tuples { - groups[i] = resources.Group{ - Id: &t.Target.ID, - Name: t.Target.ID, + + groups := make([]resources.Group, 0, len(tuples)) + for _, t := range tuples { + dbGroup, err := s.jimm.GetGroupByUUID(ctx, user, t.Target.ID) + if err != nil { + // Handle the case where the group was removed from the DB but a lingering OpenFGA tuple still exists. + // Don't return an error as that would prevent a user from viewing their groups, instead drop the group from the result. + if errors.ErrorCode(err) == errors.CodeNotFound { + continue + } + return nil, err } + groups = append(groups, resources.Group{ + Id: &t.Target.ID, + Name: dbGroup.Name, + }) } + originalToken := filter.Token() return &resources.PaginatedResponse[resources.Group]{ Data: groups, diff --git a/internal/jimmhttp/rebac_admin/identities_integration_test.go b/internal/jimmhttp/rebac_admin/identities_integration_test.go index ae5c775be..b15165909 100644 --- a/internal/jimmhttp/rebac_admin/identities_integration_test.go +++ b/internal/jimmhttp/rebac_admin/identities_integration_test.go @@ -10,6 +10,7 @@ import ( "github.com/juju/names/v5" gc "gopkg.in/check.v1" + "github.com/canonical/jimm/v3/internal/dbmodel" "github.com/canonical/jimm/v3/internal/jimmhttp/rebac_admin" "github.com/canonical/jimm/v3/internal/openfga" ofganames "github.com/canonical/jimm/v3/internal/openfga/names" @@ -103,7 +104,8 @@ func (s *identitiesSuite) TestIdentityGetGroups(c *gc.C) { c.Assert(err, gc.IsNil) token = *groups.Next.PageToken for j := 0; j < len(groups.Data); j++ { - c.Assert(groups.Data[j].Name, gc.Equals, groupTags[i+j].Id()) + c.Assert(groups.Data[j].Name, gc.Equals, fmt.Sprintf("group-test%d", i+j)) + c.Assert(groupTags[j].Id(), gc.Matches, `\w*-\w*-\w*-\w*-\w*`) } if *groups.Next.PageToken == "" { break @@ -111,6 +113,45 @@ func (s *identitiesSuite) TestIdentityGetGroups(c *gc.C) { } } +// TestGetIdentityGroupsWithDeletedDbGroup tests the behaviour +// of GetIdentityGroups when a tuple lingers in OpenFGA but the group +// has been removed from the database. +func (s *identitiesSuite) TestGetIdentityGroupsWithDeletedDbGroup(c *gc.C) { + ctx := context.Background() + ctx = rebac_handlers.ContextWithIdentity(ctx, s.AdminUser) + identitySvc := rebac_admin.NewidentitiesService(s.JIMM) + username := s.AdminUser.Name + + group1 := s.AddGroup(c, "group1") + group2 := s.AddGroup(c, "group2") + + baseTuple := openfga.Tuple{ + Object: ofganames.ConvertTag(s.AdminUser.ResourceTag()), + Relation: ofganames.MemberRelation, + } + group1Access := baseTuple + group1Access.Target = ofganames.ConvertTag(group1) + group2Access := baseTuple + group2Access.Target = ofganames.ConvertTag(group2) + + err := s.JIMM.OpenFGAClient.AddRelation(ctx, group1Access, group2Access) + c.Assert(err, gc.IsNil) + + groups, err := identitySvc.GetIdentityGroups(ctx, username, &resources.GetIdentitiesItemGroupsParams{}) + c.Assert(err, gc.IsNil) + c.Assert(groups.Data, gc.HasLen, 2) + + groupToDelete := dbmodel.GroupEntry{Name: "group2"} + err = s.JIMM.Database.GetGroup(ctx, &groupToDelete) + c.Assert(err, gc.IsNil) + err = s.JIMM.Database.RemoveGroup(ctx, &groupToDelete) + c.Assert(err, gc.IsNil) + + groups, err = identitySvc.GetIdentityGroups(ctx, username, &resources.GetIdentitiesItemGroupsParams{}) + c.Assert(err, gc.IsNil) + c.Assert(groups.Data, gc.HasLen, 1) +} + // TestIdentityEntitlements tests the listing of entitlements for a specific identityId. // Setup: add controllers, models to a user and add the user to a group. func (s *identitiesSuite) TestIdentityEntitlements(c *gc.C) { diff --git a/internal/jimmhttp/rebac_admin/identities_test.go b/internal/jimmhttp/rebac_admin/identities_test.go index 09daf63df..c93687eed 100644 --- a/internal/jimmhttp/rebac_admin/identities_test.go +++ b/internal/jimmhttp/rebac_admin/identities_test.go @@ -148,7 +148,7 @@ func TestGetIdentityGroups(t *testing.T) { testTuple := openfga.Tuple{ Object: &ofga.Entity{Kind: "user", ID: "foo"}, Relation: ofga.Relation("member"), - Target: &ofga.Entity{Kind: "group", ID: "my-group"}, + Target: &ofga.Entity{Kind: "group", ID: "my-group-id"}, } jimm := jimmtest.JIMM{ FetchIdentity_: func(ctx context.Context, username string) (*openfga.User, error) { @@ -162,6 +162,11 @@ func TestGetIdentityGroups(t *testing.T) { return []openfga.Tuple{testTuple}, "continuation-token", listTuplesErr }, }, + GroupService: mocks.GroupService{ + GetGroupByUUID_: func(ctx context.Context, user *openfga.User, uuid string) (*dbmodel.GroupEntry, error) { + return &dbmodel.GroupEntry{Name: "fake-group-name"}, nil + }, + }, } user := openfga.User{} ctx := context.Background() @@ -176,6 +181,8 @@ func TestGetIdentityGroups(t *testing.T) { c.Assert(err, qt.IsNil) c.Assert(res, qt.IsNotNil) c.Assert(res.Data, qt.HasLen, 1) + c.Assert(*res.Data[0].Id, qt.Equals, "my-group-id") + c.Assert(res.Data[0].Name, qt.Equals, "fake-group-name") c.Assert(*res.Next.PageToken, qt.Equals, "continuation-token") listTuplesErr = errors.New("foo")