diff --git a/services/graph/pkg/identity/ldap.go b/services/graph/pkg/identity/ldap.go index 398fabaca..e0f87bab5 100644 --- a/services/graph/pkg/identity/ldap.go +++ b/services/graph/pkg/identity/ldap.go @@ -389,10 +389,7 @@ func (i *LDAP) GetUser(ctx context.Context, nameOrID string, queryParam url.Valu return nil, err } if len(userGroups) > 0 { - groups := make([]libregraph.Group, 0, len(userGroups)) - for _, g := range userGroups { - groups = append(groups, *i.createGroupModelFromLDAP(g)) - } + groups := i.groupsFromLDAPEntries(userGroups) u.MemberOf = groups } } @@ -450,10 +447,7 @@ func (i *LDAP) GetUsers(ctx context.Context, queryParam url.Values) ([]*libregra if expand == "" { expand = "false" } - groups := make([]libregraph.Group, 0, len(userGroups)) - for _, g := range userGroups { - groups = append(groups, *i.createGroupModelFromLDAP(g)) - } + groups := i.groupsFromLDAPEntries(userGroups) u.MemberOf = groups } } @@ -482,7 +476,10 @@ func (i *LDAP) GetGroup(ctx context.Context, nameOrID string, queryParam url.Val } sel := strings.Split(queryParam.Get("$select"), ",") exp := strings.Split(queryParam.Get("$expand"), ",") - g := i.createGroupModelFromLDAP(e) + var g *libregraph.Group + if g = i.createGroupModelFromLDAP(e); g == nil { + return nil, errorcode.New(errorcode.ItemNotFound, "not found") + } if slices.Contains(sel, "members") || slices.Contains(exp, "members") { members, err := i.GetGroupMembers(ctx, *g.Id) if err != nil { @@ -635,10 +632,13 @@ func (i *LDAP) GetGroups(ctx context.Context, queryParam url.Values) ([]*libregr groups := make([]*libregraph.Group, 0, len(res.Entries)) + var g *libregraph.Group for _, e := range res.Entries { sel := strings.Split(queryParam.Get("$select"), ",") exp := strings.Split(queryParam.Get("$expand"), ",") - g := i.createGroupModelFromLDAP(e) + if g = i.createGroupModelFromLDAP(e); g == nil { + continue + } if slices.Contains(sel, "members") || slices.Contains(exp, "members") { members, err := i.GetGroupMembers(ctx, *g.Id) if err != nil { @@ -862,10 +862,27 @@ func (i *LDAP) createUserModelFromLDAP(e *ldap.Entry) *libregraph.User { } func (i *LDAP) createGroupModelFromLDAP(e *ldap.Entry) *libregraph.Group { - return &libregraph.Group{ - DisplayName: pointerOrNil(e.GetEqualFoldAttributeValue(i.groupAttributeMap.name)), - Id: pointerOrNil(e.GetEqualFoldAttributeValue(i.groupAttributeMap.id)), + name := e.GetEqualFoldAttributeValue(i.groupAttributeMap.name) + id := e.GetEqualFoldAttributeValue(i.groupAttributeMap.id) + + if id != "" && name != "" { + return &libregraph.Group{ + DisplayName: &name, + Id: &id, + } } + i.logger.Warn().Str("dn", e.DN).Msg("Group is missing name or id") + return nil +} + +func (i *LDAP) groupsFromLDAPEntries(e []*ldap.Entry) []libregraph.Group { + groups := make([]libregraph.Group, 0, len(e)) + for _, g := range e { + if grp := i.createGroupModelFromLDAP(g); grp != nil { + groups = append(groups, *grp) + } + } + return groups } func pointerOrNil(val string) *string { diff --git a/services/graph/pkg/identity/ldap_test.go b/services/graph/pkg/identity/ldap_test.go index 7db012edb..aa3fc790a 100644 --- a/services/graph/pkg/identity/ldap_test.go +++ b/services/graph/pkg/identity/ldap_test.go @@ -45,6 +45,10 @@ var groupEntry = ldap.NewEntry("cn=group", "cn": {"group"}, "entryuuid": {"abcd-defg"}, }) +var invalidGroupEntry = ldap.NewEntry("cn=invalid", + map[string][]string{ + "cn": {"invalid"}, + }) var logger = log.NewLogger(log.Level("debug")) @@ -266,15 +270,32 @@ func TestGetGroup(t *testing.T) { t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) } - // Mock a valid Search Result + // Mock an invalid Search Result lm = &mocks.Client{} - lm.On("Search", mock.Anything, mock.Anything, mock.Anything, mock.Anything, - mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). - Return(&ldap.SearchResult{ - Entries: []*ldap.Entry{groupEntry}, - }, nil) + lm.On("Search", mock.Anything).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{invalidGroupEntry}, + }, nil) b, _ = getMockedBackend(lm, lconfig, &logger) g, err := b.GetGroup(context.Background(), "group", nil) + if err == nil || err.Error() != "itemNotFound" { + t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) + } + g, err = b.GetGroup(context.Background(), "group", queryParamExpand) + if err == nil || err.Error() != "itemNotFound" { + t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) + } + g, err = b.GetGroup(context.Background(), "group", queryParamSelect) + if err == nil || err.Error() != "itemNotFound" { + t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) + } + + // Mock a valid Search Result + lm = &mocks.Client{} + lm.On("Search", mock.Anything).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{groupEntry}, + }, nil) + b, _ = getMockedBackend(lm, lconfig, &logger) + g, err = b.GetGroup(context.Background(), "group", nil) if err != nil { t.Errorf("Expected GetGroup to succeed. Got %s", err.Error()) } else if *g.Id != groupEntry.GetEqualFoldAttributeValue(b.groupAttributeMap.id) {