Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

preserve Identity Center imported group member label #48785

Merged
merged 6 commits into from
Nov 13, 2024
Merged

preserve Identity Center imported group member label #48785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b19e71f
preserve Identity Center imported group member label
flyinghermit Nov 12, 2024
8f72b60
test
flyinghermit Nov 12, 2024
d7e6ba0
preserve label on udpate/upsert member methods
flyinghermit Nov 12, 2024
3aeea17
address review comments
flyinghermit Nov 12, 2024
e5cfdd1
error check one liner, remove unnecessary return
flyinghermit Nov 13, 2024
e84e474
Merge branch 'master' into sshah/group-member-import
flyinghermit Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions lib/services/local/access_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
accesslistv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/accesslist/v1"
"github.com/gravitational/teleport/api/types"
"github.com/gravitational/teleport/api/types/accesslist"
"github.com/gravitational/teleport/api/types/common"
"github.com/gravitational/teleport/api/types/header"
"github.com/gravitational/teleport/entitlements"
"github.com/gravitational/teleport/lib/accesslists"
Expand Down Expand Up @@ -544,6 +545,11 @@ func (a *AccessListService) UpsertAccessListMember(ctx context.Context, member *
if err != nil {
return trace.Wrap(err)
}
existingMember, err := a.memberService.GetResource(ctx, member.GetName())
if err != nil && !trace.IsNotFound(err) {
return trace.Wrap(err)
}
keepAWSIdentityCenterLabels(existingMember, member)

if err := accesslists.ValidateAccessListMember(ctx, memberList, member, &accessListAndMembersGetter{a.service, a.memberService}); err != nil {
return trace.Wrap(err)
Expand Down Expand Up @@ -575,6 +581,11 @@ func (a *AccessListService) UpdateAccessListMember(ctx context.Context, member *
if err != nil {
return trace.Wrap(err)
}
existingMember, err := a.memberService.GetResource(ctx, member.GetName())
if err != nil && !trace.IsNotFound(err) {
return trace.Wrap(err)
}
keepAWSIdentityCenterLabels(existingMember, member)

if err := accesslists.ValidateAccessListMember(ctx, memberList, member, &accessListAndMembersGetter{a.service, a.memberService}); err != nil {
return trace.Wrap(err)
Expand Down Expand Up @@ -732,6 +743,7 @@ func (a *AccessListService) UpsertAccessListWithMembers(ctx context.Context, acc
if existingMember.Spec.Reason != "" {
newMember.Spec.Reason = existingMember.Spec.Reason
}
keepAWSIdentityCenterLabels(existingMember, newMember)
newMember.Spec.AddedBy = existingMember.Spec.AddedBy

// Compare members and update if necessary.
Expand Down Expand Up @@ -1029,3 +1041,17 @@ func (a *AccessListService) VerifyAccessListCreateLimit(ctx context.Context, tar
const limitReachedMessage = "cluster has reached its limit for creating access lists, please contact the cluster administrator"
return trace.AccessDenied(limitReachedMessage)
}

// keepAWSIdentityCenterLabels preserves member labels if
// it originated from AWS Identity Center plugin.
// The Web UI does not currently preserve metadata labels so this function should be called
// in every update/upsert member calls.
// Remove this function once https://github.com/gravitational/teleport.e/issues/5415 is addressed.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, wouldn't it make more sense to update the memberToAccessListMember function that you linked in that ticket to keep the labels when creating a new member then?

Copy link
Contributor Author

@flyinghermit flyinghermit Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did look at it before creating the ticket and its not going to be a trivial change. I don't even know if its a feature or a bug cause the way it is currently implemented, its a single API where we upsert entire access list, even if its just a single member addition/removal https://github.com/gravitational/teleport.e/blob/master/lib/web/plugin.go#L285.

Preserving older state is not something exclusively done for the identity center. See prior work on https://github.com/gravitational/teleport/blob/master/lib/services/local/access_list.go#L729C1-L735C58. Looks like we settled on with the current implementation? Anyway, I think updating that API to handle label and Idmetadata name is best tackled separately.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand we replace the list, but from the ticket description the impression I got was that we just do not add member.Metadata.Labels in that function. Is that not the case?

I don't have objection to keep this implementation for now but trying to understand why passing the labels to the newly created member in that function does not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not being clear. Another attempt below:

that we just do not add member.Metadata.Labels in that function. Is that not the case?

Yup correct. But that is also due to the fact that the memberToAccessListMember function does not have access to the metadata label because we do not send them to the UI in GET Access List https://github.com/gravitational/teleport.e/blob/master/lib/web/ui/accesslist.go#L10C1-L25C2. And as a result the UI does not handle metadata values, nor it send them back in the upsert request. https://github.com/gravitational/teleport.e/blob/master/lib/web/ui/accesslist.go#L38C1-L42C2. Hence looses any labels applied to the member.

So besides the memberToAccessListMember function, Access List GET and PUT apis along with Web UI needs to be updated to solve that issue.

func keepAWSIdentityCenterLabels(old, new *accesslist.AccessListMember) {
if old == nil || new == nil {
return
}
if old.Origin() == common.OriginAWSIdentityCenter {
new.Metadata.Labels = old.GetAllLabels()
}
}
46 changes: 46 additions & 0 deletions lib/services/local/access_list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"github.com/stretchr/testify/require"

"github.com/gravitational/teleport/api/types/accesslist"
"github.com/gravitational/teleport/api/types/common"
"github.com/gravitational/teleport/api/types/header"
"github.com/gravitational/teleport/api/types/trait"
"github.com/gravitational/teleport/entitlements"
Expand Down Expand Up @@ -662,6 +663,51 @@ func TestAccessListMembersCRUD(t *testing.T) {
require.ErrorIs(t, err, trace.NotFound("access_list %q doesn't exist", accessList2.GetName()))
}

func TestUpsertAndUpdateAccessListWithMembers_PreservesIdentityCenterLablesForExistingMembers(t *testing.T) {
ctx := context.Background()
clock := clockwork.NewFakeClock()
mem, err := memory.New(memory.Config{
Context: ctx,
Clock: clock,
})
require.NoError(t, err)
service := newAccessListService(t, mem, clock, true /* igsEnabled */)

accessList1 := newAccessList(t, "accessList1", clock)
_, err = service.UpsertAccessList(ctx, accessList1)
require.NoError(t, err)
accessList1Member1 := newAccessListMember(t, accessList1.GetName(), "aws-ic-user")
accessList1Member1.SetOrigin(common.OriginAWSIdentityCenter)
accessList1Member1.Metadata.Labels["foo"] = "bar"

_, err = service.UpsertAccessListMember(ctx, accessList1Member1)
require.NoError(t, err)

member, err := service.GetAccessListMember(ctx, accessList1.GetName(), accessList1Member1.GetName())
require.NoError(t, err)
require.Empty(
t,
cmp.Diff(
accessList1Member1,
member,
cmpopts.IgnoreFields(header.Metadata{}, "Revision"),
cmpopts.IgnoreFields(accesslist.AccessListMemberSpec{}, "Joined"),
))

dupeMemberButWithoutOriginLabel := newAccessListMember(t, accessList1.GetName(), "aws-ic-user")
_, updatedMembers, err := service.UpsertAccessListWithMembers(ctx, accessList1, []*accesslist.AccessListMember{dupeMemberButWithoutOriginLabel})
require.NoError(t, err)
require.Equal(t, "bar", updatedMembers[0].GetMetadata().Labels["foo"])

updatedMember, err := service.UpdateAccessListMember(ctx, dupeMemberButWithoutOriginLabel)
require.NoError(t, err)
require.Equal(t, "bar", updatedMember.GetMetadata().Labels["foo"])

upsertedMember, err := service.UpdateAccessListMember(ctx, dupeMemberButWithoutOriginLabel)
require.NoError(t, err)
require.Equal(t, "bar", upsertedMember.GetMetadata().Labels["foo"])
}

func TestAccessListReviewCRUD(t *testing.T) {
ctx := context.Background()
clock := clockwork.NewFakeClock()
Expand Down
Loading