Return the group name when listing a user's groups. #1422
Conversation
kian99
force-pushed
the
fix-identity-groups-response
branch
from
bed2563
to
27253b5
Compare
| 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 show the group as "removed". | ||
| dbGroup.Name = "(removed)" |
There was a problem hiding this comment.
thinking out loud...
if the group is missing from the db it won't be listed in the /groups | group/{id} endpoint, so showing the id in this endpoint doesn't make sense, because it can't be resolved if the user clicks on it.
So I would remove these entries missing from postgres, so the user doesn't see "dangling" uuid.
Just a continue without returning any error is fine.
There was a problem hiding this comment.
I agree, dropping the result does seem like the better thing to do. I'll wait for another review to chime in and if we agree, I'll change it.
| 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 show the group as "removed". |
There was a problem hiding this comment.
How is the group shown as "removed"? We just call .First
There was a problem hiding this comment.
i think cleanup could possibly be a topic in its own right. we leave a lot of tuples in openfga that refer to a deleted entity in jimm. i think a spec on this would be very welcome, if anybody has time.
There was a problem hiding this comment.
Comment was outdated, we just drop the group from the result.
| 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 show the group as "removed". | ||
| if errors.ErrorCode(err) == errors.CodeNotFound { |
There was a problem hiding this comment.
Should we be leaving lingering tuples? Or would .GetGroupsByUUIDs be better in the JIMM layer with tuple cleanup? (Really not sure here just opening up for discussion)
There was a problem hiding this comment.
We shouldn't be leaving lingering tuples, but that's a separate issue. This is just handling the case that, for some reason, there is a leftover tuple but no group in the database.
There was a problem hiding this comment.
Not sure what you mean here but perhaps explain in tomorrows standup or offline in element?
| 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 show the group as "removed". |
There was a problem hiding this comment.
i think cleanup could possibly be a topic in its own right. we leave a lot of tuples in openfga that refer to a deleted entity in jimm. i think a spec on this would be very welcome, if anybody has time.
| c.Assert(err, gc.IsNil) | ||
| c.Assert(groups.Data, gc.HasLen, 2) | ||
|
|
||
| groupToDelete := dbmodel.GroupEntry{Name: "group2"} |
There was a problem hiding this comment.
i don't think you need to get group2 again.. you have its tag on L126 (which contains the id).
There was a problem hiding this comment.
I thought so too, but the delete method requires the ID or UUID.
| 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. |
There was a problem hiding this comment.
We need to handle the lingering tuples... Should have a discussion on this
Description
This PR fixes the response from the rebac admin's "GetIdentityGroups" endpoint. Currently we return a group name in place of the group ID. One issue we face is that the group name may no longer exist in the DB but an OpenFGA may still exist. To handle this case I considered:
I've opted for option 3. The first would prevent the user from viewing their groups and the second would hide the issue.Changed to option 2 to drop the result since it is not useful to the user.
Fixes #1418.
Engineering checklist
Check only items that apply