From 5065a67891cde72ac3d38cd8ba5977da3a711725 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Mon, 2 Mar 2026 17:53:37 +0100 Subject: [PATCH 1/5] feat(graph/edu): Add externalID user property --- .../deployments/multi-tenancy/initialize_users.go | 4 ++++ services/graph/pkg/identity/ldap_education_user.go | 14 +++++++++++++- .../graph/pkg/identity/ldap_education_user_test.go | 1 + 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/devtools/deployments/multi-tenancy/initialize_users.go b/devtools/deployments/multi-tenancy/initialize_users.go index 9c64182b44..b376e3476a 100644 --- a/devtools/deployments/multi-tenancy/initialize_users.go +++ b/devtools/deployments/multi-tenancy/initialize_users.go @@ -31,11 +31,13 @@ var demoTenants = []tenantWithUsers{ DisplayName: libregraph.PtrString("Dennis Ritchie"), OnPremisesSamAccountName: libregraph.PtrString("dennis"), Mail: libregraph.PtrString("dennis@example.org"), + ExternalId: libregraph.PtrString("ExternalID1"), }, { DisplayName: libregraph.PtrString("Grace Hopper"), OnPremisesSamAccountName: libregraph.PtrString("grace"), Mail: libregraph.PtrString("grace@example.org"), + ExternalId: libregraph.PtrString("ExternalID2"), }, }, }, @@ -49,11 +51,13 @@ var demoTenants = []tenantWithUsers{ DisplayName: libregraph.PtrString("Albert Einstein"), OnPremisesSamAccountName: libregraph.PtrString("einstein"), Mail: libregraph.PtrString("einstein@example.org"), + ExternalId: libregraph.PtrString("ExternalID3"), }, { DisplayName: libregraph.PtrString("Marie Curie"), OnPremisesSamAccountName: libregraph.PtrString("marie"), Mail: libregraph.PtrString("marie@example.org"), + ExternalId: libregraph.PtrString("ExternalID4"), }, }, }, diff --git a/services/graph/pkg/identity/ldap_education_user.go b/services/graph/pkg/identity/ldap_education_user.go index c2700195ed..9fb33f78fb 100644 --- a/services/graph/pkg/identity/ldap_education_user.go +++ b/services/graph/pkg/identity/ldap_education_user.go @@ -12,11 +12,13 @@ import ( type educationUserAttributeMap struct { primaryRole string + externalID string } func newEducationUserAttributeMap() educationUserAttributeMap { return educationUserAttributeMap{ primaryRole: "userClass", + externalID: "openCloudEducationExternalId", } } @@ -33,7 +35,7 @@ func (i *LDAP) CreateEducationUser(ctx context.Context, user libregraph.Educatio return nil, err } - if err := i.conn.Add(ar); err != nil { + if err = i.conn.Add(ar); err != nil { var lerr *ldap.Error logger.Debug().Err(err).Msg("error adding user") if errors.As(err, &lerr) { @@ -118,6 +120,7 @@ func (i *LDAP) UpdateEducationUser(ctx context.Context, nameOrID string, user li i.userAttributeMap.givenName: user.GetGivenName(), i.userAttributeMap.userType: user.GetUserType(), i.educationConfig.userAttributeMap.primaryRole: user.GetPrimaryRole(), + i.educationConfig.userAttributeMap.externalID: user.GetExternalId(), } for attribute, value := range properties { @@ -277,6 +280,10 @@ func (i *LDAP) userToEducationUser(user libregraph.User, e *ldap.Entry) *libregr if primaryRole := e.GetEqualFoldAttributeValue(i.educationConfig.userAttributeMap.primaryRole); primaryRole != "" { eduUser.SetPrimaryRole(primaryRole) } + + if externalID := e.GetEqualFoldAttributeValue(i.educationConfig.userAttributeMap.externalID); externalID != "" { + eduUser.SetExternalId(externalID) + } } return eduUser @@ -286,6 +293,10 @@ func (i *LDAP) educationUserToLDAPAttrValues(user libregraph.EducationUser, attr if role, ok := user.GetPrimaryRoleOk(); ok { attrs[i.educationConfig.userAttributeMap.primaryRole] = []string{*role} } + + if externalID, ok := user.GetExternalIdOk(); ok { + attrs[i.educationConfig.userAttributeMap.externalID] = []string{*externalID} + } attrs["objectClass"] = append(attrs["objectClass"], i.educationConfig.userObjectClass) return attrs, nil } @@ -326,6 +337,7 @@ func (i *LDAP) getEducationUserAttrTypes() []string { i.userAttributeMap.userType, i.userAttributeMap.identities, i.educationConfig.userAttributeMap.primaryRole, + i.educationConfig.userAttributeMap.externalID, i.educationConfig.memberOfSchoolAttribute, } } diff --git a/services/graph/pkg/identity/ldap_education_user_test.go b/services/graph/pkg/identity/ldap_education_user_test.go index 5fbefa5d82..bfe047d9b6 100644 --- a/services/graph/pkg/identity/ldap_education_user_test.go +++ b/services/graph/pkg/identity/ldap_education_user_test.go @@ -22,6 +22,7 @@ var eduUserAttrs = []string{ "userTypeAttribute", "openCloudExternalIdentity", "userClass", + "openCloudEducationExternalId", "openCloudMemberOfSchool", } From 6f404096cee34d2564338675c5fc422289249472 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Tue, 3 Mar 2026 16:22:00 +0100 Subject: [PATCH 2/5] feat(graph/education): Add support of 'eq' filters on users This adds support of simple OData filters on the 'education/users' endpoint. Filters of the type '$filter= eq ' are supported now for the following educationUser properties: "displayname", "mail", "userType", "primaryRole" and "externalId" Closes: #1599 --- services/graph/pkg/identity/backend.go | 2 + services/graph/pkg/identity/err_education.go | 5 ++ .../graph/pkg/identity/ldap_education_user.go | 54 ++++++++++++++ .../pkg/identity/ldap_education_user_test.go | 30 ++++++-- .../pkg/identity/mocks/education_backend.go | 74 +++++++++++++++++++ .../graph/pkg/service/v0/educationuser.go | 68 +++++++++++++++-- .../pkg/service/v0/educationuser_test.go | 12 +++ services/graph/pkg/service/v0/users_filter.go | 7 +- 8 files changed, 236 insertions(+), 16 deletions(-) diff --git a/services/graph/pkg/identity/backend.go b/services/graph/pkg/identity/backend.go index 93684311d5..dc1f2b1121 100644 --- a/services/graph/pkg/identity/backend.go +++ b/services/graph/pkg/identity/backend.go @@ -107,6 +107,8 @@ type EducationBackend interface { GetEducationUser(ctx context.Context, nameOrID string) (*libregraph.EducationUser, error) // GetEducationUsers lists all education users GetEducationUsers(ctx context.Context) ([]*libregraph.EducationUser, error) + // FilterEducationUsersByAttribute list all education users where and attribute matches a value, e.g. all users with a given externalid + FilterEducationUsersByAttribute(ctx context.Context, attr, value string) ([]*libregraph.EducationUser, error) // GetEducationClassTeachers returns the EducationUser teachers for an EducationClass GetEducationClassTeachers(ctx context.Context, classID string) ([]*libregraph.EducationUser, error) diff --git a/services/graph/pkg/identity/err_education.go b/services/graph/pkg/identity/err_education.go index 08609d2749..8c0e048534 100644 --- a/services/graph/pkg/identity/err_education.go +++ b/services/graph/pkg/identity/err_education.go @@ -119,6 +119,11 @@ func (i *ErrEducationBackend) GetEducationUsers(ctx context.Context) ([]*libregr return nil, errNotImplemented } +// FilterEducationUsersByAttribute implements the EducationBackend interface for the ErrEducationBackend backend. +func (i *ErrEducationBackend) FilterEducationUsersByAttribute(ctx context.Context, attr, value string) ([]*libregraph.EducationUser, error) { + return nil, errNotImplemented +} + // GetEducationClassTeachers implements the EducationBackend interface for the ErrEducationBackend backend. func (i *ErrEducationBackend) GetEducationClassTeachers(ctx context.Context, classID string) ([]*libregraph.EducationUser, error) { return nil, errNotImplemented diff --git a/services/graph/pkg/identity/ldap_education_user.go b/services/graph/pkg/identity/ldap_education_user.go index 9fb33f78fb..5a5bf0bb66 100644 --- a/services/graph/pkg/identity/ldap_education_user.go +++ b/services/graph/pkg/identity/ldap_education_user.go @@ -252,6 +252,60 @@ func (i *LDAP) GetEducationUsers(ctx context.Context) ([]*libregraph.EducationUs return users, nil } +func (i *LDAP) FilterEducationUsersByAttribute(ctx context.Context, attr, value string) ([]*libregraph.EducationUser, error) { + logger := i.logger.SubloggerWithRequestID(ctx).With().Str("func", "FilterEducationUsersByAttribute").Logger() + logger.Debug().Str("backend", "ldap").Str("attribute", attr).Str("value", value).Msg("") + + var ldapAttr string + switch attr { + case "displayname": + ldapAttr = i.userAttributeMap.displayName + case "mail": + ldapAttr = i.userAttributeMap.mail + case "userType": + ldapAttr = i.userAttributeMap.userType + case "primaryRole": + ldapAttr = i.educationConfig.userAttributeMap.primaryRole + case "externalId": + ldapAttr = i.educationConfig.userAttributeMap.externalID + default: + return nil, errorcode.New(errorcode.InvalidRequest, fmt.Sprintf("filtering by attribute '%s' is not supported", attr)) + } + filter := fmt.Sprintf("(&%s(objectClass=%s)(%s=%s))", i.userFilter, i.educationConfig.userObjectClass, ldap.EscapeFilter(ldapAttr), ldap.EscapeFilter(value)) + + searchRequest := ldap.NewSearchRequest( + i.userBaseDN, + i.userScope, + ldap.NeverDerefAliases, 0, 0, false, + filter, + i.getEducationUserAttrTypes(), + nil, + ) + logger.Debug().Str("base", searchRequest.BaseDN). + Str("filter", searchRequest.Filter). + Int("scope", searchRequest.Scope). + Int("sizelimit", searchRequest.SizeLimit). + Interface("attributes", searchRequest.Attributes). + Msg("LDAP Search Request") + + res, err := i.conn.Search(searchRequest) + if err != nil { + return nil, errorcode.New(errorcode.ItemNotFound, err.Error()) + } + + users := make([]*libregraph.EducationUser, 0, len(res.Entries)) + + for _, e := range res.Entries { + u := i.createEducationUserModelFromLDAP(e) + // Skip invalid LDAP users + if u == nil { + continue + } + users = append(users, u) + } + return users, nil +} + func (i *LDAP) educationUserToUser(eduUser libregraph.EducationUser) *libregraph.User { user := libregraph.NewUser(*eduUser.DisplayName, *eduUser.OnPremisesSamAccountName) user.Surname = eduUser.Surname diff --git a/services/graph/pkg/identity/ldap_education_user_test.go b/services/graph/pkg/identity/ldap_education_user_test.go index bfe047d9b6..d85a73b448 100644 --- a/services/graph/pkg/identity/ldap_education_user_test.go +++ b/services/graph/pkg/identity/ldap_education_user_test.go @@ -5,12 +5,14 @@ import ( "testing" "github.com/go-ldap/ldap/v3" - "github.com/opencloud-eu/opencloud/services/graph/pkg/identity/mocks" libregraph "github.com/opencloud-eu/libre-graph-api-go" + "github.com/opencloud-eu/opencloud/services/graph/pkg/identity/mocks" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" ) +const peopleBaseDN = "ou=people,dc=test" + var eduUserAttrs = []string{ "displayname", "entryUUID", @@ -69,7 +71,7 @@ var eduUserEntryWithSchool = ldap.NewEntry("uid=user,ou=people,dc=test", }) var sr1 *ldap.SearchRequest = &ldap.SearchRequest{ - BaseDN: "ou=people,dc=test", + BaseDN: peopleBaseDN, Scope: 2, SizeLimit: 1, Filter: "(&(objectClass=openCloudEducationUser)(|(uid=abcd-defg)(entryUUID=abcd-defg)))", @@ -77,7 +79,7 @@ var sr1 *ldap.SearchRequest = &ldap.SearchRequest{ Controls: []ldap.Control(nil), } var sr2 *ldap.SearchRequest = &ldap.SearchRequest{ - BaseDN: "ou=people,dc=test", + BaseDN: peopleBaseDN, Scope: 2, SizeLimit: 1, Filter: "(&(objectClass=openCloudEducationUser)(|(uid=xxxx-xxxx)(entryUUID=xxxx-xxxx)))", @@ -164,7 +166,7 @@ func TestGetEducationUser(t *testing.T) { func TestGetEducationUsers(t *testing.T) { lm := &mocks.Client{} sr := &ldap.SearchRequest{ - BaseDN: "ou=people,dc=test", + BaseDN: peopleBaseDN, Scope: 2, SizeLimit: 0, Filter: "(objectClass=openCloudEducationUser)", @@ -179,12 +181,30 @@ func TestGetEducationUsers(t *testing.T) { assert.Nil(t, err) } +func TestFilterEducationUsersByAttr(t *testing.T) { + lm := &mocks.Client{} + sr := &ldap.SearchRequest{ + BaseDN: peopleBaseDN, + Scope: 2, + SizeLimit: 0, + Filter: "(&(objectClass=openCloudEducationUser)(openCloudEducationExternalId=id1234))", + Attributes: eduUserAttrs, + Controls: []ldap.Control(nil), + } + lm.On("Search", sr).Return(&ldap.SearchResult{Entries: []*ldap.Entry{eduUserEntry}}, nil) + b, err := getMockedBackend(lm, eduConfig, &logger) + assert.Nil(t, err) + _, err = b.FilterEducationUsersByAttribute(context.Background(), "externalId", "id1234") + lm.AssertNumberOfCalls(t, "Search", 1) + assert.Nil(t, err) +} + func TestUpdateEducationUser(t *testing.T) { lm := &mocks.Client{} b, err := getMockedBackend(lm, eduConfig, &logger) assert.Nil(t, err) userSearchReq := &ldap.SearchRequest{ - BaseDN: "ou=people,dc=test", + BaseDN: peopleBaseDN, Scope: 2, SizeLimit: 1, Filter: "(&(objectClass=openCloudEducationUser)(|(uid=testuser)(entryUUID=testuser)))", diff --git a/services/graph/pkg/identity/mocks/education_backend.go b/services/graph/pkg/identity/mocks/education_backend.go index 08e075bd2f..8933df7104 100644 --- a/services/graph/pkg/identity/mocks/education_backend.go +++ b/services/graph/pkg/identity/mocks/education_backend.go @@ -602,6 +602,80 @@ func (_c *EducationBackend_DeleteEducationUser_Call) RunAndReturn(run func(ctx c return _c } +// FilterEducationUsersByAttribute provides a mock function for the type EducationBackend +func (_mock *EducationBackend) FilterEducationUsersByAttribute(ctx context.Context, attr string, value string) ([]*libregraph.EducationUser, error) { + ret := _mock.Called(ctx, attr, value) + + if len(ret) == 0 { + panic("no return value specified for FilterEducationUsersByAttribute") + } + + var r0 []*libregraph.EducationUser + var r1 error + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) ([]*libregraph.EducationUser, error)); ok { + return returnFunc(ctx, attr, value) + } + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) []*libregraph.EducationUser); ok { + r0 = returnFunc(ctx, attr, value) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*libregraph.EducationUser) + } + } + if returnFunc, ok := ret.Get(1).(func(context.Context, string, string) error); ok { + r1 = returnFunc(ctx, attr, value) + } else { + r1 = ret.Error(1) + } + return r0, r1 +} + +// EducationBackend_FilterEducationUsersByAttribute_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'FilterEducationUsersByAttribute' +type EducationBackend_FilterEducationUsersByAttribute_Call struct { + *mock.Call +} + +// FilterEducationUsersByAttribute is a helper method to define mock.On call +// - ctx context.Context +// - attr string +// - value string +func (_e *EducationBackend_Expecter) FilterEducationUsersByAttribute(ctx interface{}, attr interface{}, value interface{}) *EducationBackend_FilterEducationUsersByAttribute_Call { + return &EducationBackend_FilterEducationUsersByAttribute_Call{Call: _e.mock.On("FilterEducationUsersByAttribute", ctx, attr, value)} +} + +func (_c *EducationBackend_FilterEducationUsersByAttribute_Call) Run(run func(ctx context.Context, attr string, value string)) *EducationBackend_FilterEducationUsersByAttribute_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 context.Context + if args[0] != nil { + arg0 = args[0].(context.Context) + } + var arg1 string + if args[1] != nil { + arg1 = args[1].(string) + } + var arg2 string + if args[2] != nil { + arg2 = args[2].(string) + } + run( + arg0, + arg1, + arg2, + ) + }) + return _c +} + +func (_c *EducationBackend_FilterEducationUsersByAttribute_Call) Return(educationUsers []*libregraph.EducationUser, err error) *EducationBackend_FilterEducationUsersByAttribute_Call { + _c.Call.Return(educationUsers, err) + return _c +} + +func (_c *EducationBackend_FilterEducationUsersByAttribute_Call) RunAndReturn(run func(ctx context.Context, attr string, value string) ([]*libregraph.EducationUser, error)) *EducationBackend_FilterEducationUsersByAttribute_Call { + _c.Call.Return(run) + return _c +} + // GetEducationClass provides a mock function for the type EducationBackend func (_mock *EducationBackend) GetEducationClass(ctx context.Context, namedOrID string) (*libregraph.EducationClass, error) { ret := _mock.Called(ctx, namedOrID) diff --git a/services/graph/pkg/service/v0/educationuser.go b/services/graph/pkg/service/v0/educationuser.go index ee23e3e2a2..8fa87c5eec 100644 --- a/services/graph/pkg/service/v0/educationuser.go +++ b/services/graph/pkg/service/v0/educationuser.go @@ -1,6 +1,8 @@ package svc import ( + "context" + "errors" "fmt" "net/http" "net/url" @@ -21,8 +23,7 @@ import ( // GetEducationUsers implements the Service interface. func (g Graph) GetEducationUsers(w http.ResponseWriter, r *http.Request) { - logger := g.logger.SubloggerWithRequestID(r.Context()) - logger.Info().Interface("query", r.URL.Query()).Msg("calling get education users") + logger := g.logger.SubloggerWithRequestID(r.Context()).With().Str("func", "GetEducationUsers").Logger() sanitizedPath := strings.TrimPrefix(r.URL.Path, "/graph/v1.0/") odataReq, err := godata.ParseRequest(r.Context(), sanitizedPath, r.URL.Query()) if err != nil { @@ -31,12 +32,36 @@ func (g Graph) GetEducationUsers(w http.ResponseWriter, r *http.Request) { return } - logger.Debug().Interface("query", r.URL.Query()).Msg("calling get education users on backend") - users, err := g.identityEducationBackend.GetEducationUsers(r.Context()) - if err != nil { - logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get education users from backend") - errorcode.RenderError(w, r, err) - return + var users []*libregraph.EducationUser + if odataReq.Query.Filter != nil { + attr, value, err := g.getEqualityFilter(r.Context(), odataReq) + if err != nil { + logger.Debug().Err(err).Str("filter", odataReq.Query.Filter.RawValue).Msg("failed to parse filter") + var errcode errorcode.Error + var godataerr *godata.GoDataError + switch { + case errors.As(err, &errcode): + errcode.Render(w, r) + case errors.As(err, &godataerr): + errorcode.GeneralException.Render(w, r, godataerr.ResponseCode, err.Error()) + default: + errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error()) + } + return + } + users, err = g.identityEducationBackend.FilterEducationUsersByAttribute(r.Context(), attr, value) + if err != nil { + logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get education users from backend") + errorcode.RenderError(w, r, err) + return + } + } else { + users, err = g.identityEducationBackend.GetEducationUsers(r.Context()) + if err != nil { + logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get education users from backend") + errorcode.RenderError(w, r, err) + return + } } users, err = sortEducationUsers(odataReq, users) @@ -387,3 +412,30 @@ func sortEducationUsers(req *godata.GoDataRequest, users []*libregraph.Education } return users, nil } + +func (g Graph) getEqualityFilter(ctx context.Context, req *godata.GoDataRequest) (string, string, error) { + logger := g.logger.SubloggerWithRequestID(ctx) + + root := req.Query.Filter.Tree + + if root.Token.Type != godata.ExpressionTokenLogical { + logger.Debug().Str("filter", req.Query.Filter.RawValue).Msg(unsupportedFilter) + return "", "", unsupportedFilterError() + } + if root.Token.Value != "eq" { + logger.Debug().Str("filter", req.Query.Filter.RawValue).Msg(unsupportedFilter) + return "", "", unsupportedFilterError() + } + if len(root.Children) != 2 { + logger.Debug().Str("filter", req.Query.Filter.RawValue).Msg(unsupportedFilter) + return "", "", unsupportedFilterError() + } + if root.Children[0].Token.Type != godata.ExpressionTokenLiteral || root.Children[1].Token.Type != godata.ExpressionTokenString { + logger.Debug().Str("filter", req.Query.Filter.RawValue).Msg(unsupportedFilter) + return "", "", unsupportedFilterError() + } + + // unquote + value := strings.Trim(root.Children[1].Token.Value, "'") + return root.Children[0].Token.Value, value, nil +} diff --git a/services/graph/pkg/service/v0/educationuser_test.go b/services/graph/pkg/service/v0/educationuser_test.go index 292cbd824f..077f270bb1 100644 --- a/services/graph/pkg/service/v0/educationuser_test.go +++ b/services/graph/pkg/service/v0/educationuser_test.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "net/http/httptest" + "net/url" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" userv1beta1 "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" @@ -202,6 +203,17 @@ var _ = Describe("EducationUsers", func() { Expect(rr.Code).To(Equal(http.StatusBadRequest)) }) + When("used with a filter", func() { + It("fails with an unsupported filter ", func() { + svc.GetEducationUsers(rr, httptest.NewRequest(http.MethodGet, "/graph/v1.0/education/users?$filter="+url.QueryEscape("displayName ne 'test'"), nil)) + Expect(rr.Code).To(Equal(http.StatusNotImplemented)) + }) + It("calls the backend with the filter", func() { + identityEducationBackend.On("FilterEducationUsersByAttribute", mock.Anything, "externalId", "id1234").Return([]*libregraph.EducationUser{}, nil) + svc.GetEducationUsers(rr, httptest.NewRequest(http.MethodGet, "/graph/v1.0/education/users?$filter="+url.QueryEscape("externalId eq 'id1234'"), nil)) + Expect(rr.Code).To(Equal(http.StatusOK)) + }) + }) }) Describe("GetEducationUser", func() { diff --git a/services/graph/pkg/service/v0/users_filter.go b/services/graph/pkg/service/v0/users_filter.go index 0fc0b1c27b..0232e06f2e 100644 --- a/services/graph/pkg/service/v0/users_filter.go +++ b/services/graph/pkg/service/v0/users_filter.go @@ -5,14 +5,15 @@ import ( "strings" "github.com/CiscoM31/godata" + libregraph "github.com/opencloud-eu/libre-graph-api-go" settingsmsg "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/messages/settings/v0" settingssvc "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/services/settings/v0" - libregraph "github.com/opencloud-eu/libre-graph-api-go" ) const ( appRoleID = "appRoleId" appRoleAssignments = "appRoleAssignments" + unsupportedFilter = "unsupported filter" ) func invalidFilterError() error { @@ -20,7 +21,7 @@ func invalidFilterError() error { } func unsupportedFilterError() error { - return godata.NotImplementedError("unsupported filter") + return godata.NotImplementedError(unsupportedFilter) } func (g Graph) applyUserFilter(ctx context.Context, req *godata.GoDataRequest, root *godata.ParseNode) (users []*libregraph.User, err error) { @@ -38,7 +39,7 @@ func (g Graph) applyUserFilter(ctx context.Context, req *godata.GoDataRequest, r case godata.ExpressionTokenFunc: return g.applyFilterFunction(ctx, req, root) } - logger.Debug().Str("filter", req.Query.Filter.RawValue).Msg("filter is not supported") + logger.Debug().Str("filter", req.Query.Filter.RawValue).Msg(unsupportedFilter) return users, unsupportedFilterError() } From 020a37b017164dcd7a44929414174922f8287148 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 4 Mar 2026 13:47:54 +0100 Subject: [PATCH 3/5] feat(graph): replace externalId school lookup with OData $filter support Remove the ability to look up schools by externalId directly (from LDAP filters, duplicate checks, and the EducationBackend interface). This approach was somewhat unclean, we shouldn't add more an more attributes as keys for direct lookup. Instead, expose externalId filtering via the OData $filter query parameter on GET /education/schools, following the same pattern as for education users. Related: #1598 --- services/graph/pkg/identity/backend.go | 2 + services/graph/pkg/identity/err_education.go | 5 + .../pkg/identity/ldap_education_school.go | 127 +++++++++++------- .../identity/ldap_education_school_test.go | 26 ++-- .../pkg/identity/mocks/education_backend.go | 74 ++++++++++ .../graph/pkg/service/v0/educationschools.go | 35 ++++- .../pkg/service/v0/educationschools_test.go | 13 ++ 7 files changed, 212 insertions(+), 70 deletions(-) diff --git a/services/graph/pkg/identity/backend.go b/services/graph/pkg/identity/backend.go index dc1f2b1121..342af7ade9 100644 --- a/services/graph/pkg/identity/backend.go +++ b/services/graph/pkg/identity/backend.go @@ -68,6 +68,8 @@ type EducationBackend interface { GetEducationSchool(ctx context.Context, nameOrID string) (*libregraph.EducationSchool, error) // GetEducationSchools lists all schools GetEducationSchools(ctx context.Context) ([]*libregraph.EducationSchool, error) + // FilterEducationSchoolsByAttribute list all schools where an attribute matches a value, e.g. all schools with a given externalId + FilterEducationSchoolsByAttribute(ctx context.Context, attr, value string) ([]*libregraph.EducationSchool, error) // UpdateEducationSchool updates attributes of a school UpdateEducationSchool(ctx context.Context, numberOrID string, school libregraph.EducationSchool) (*libregraph.EducationSchool, error) // GetEducationSchoolUsers lists all members of a school diff --git a/services/graph/pkg/identity/err_education.go b/services/graph/pkg/identity/err_education.go index 8c0e048534..4138299035 100644 --- a/services/graph/pkg/identity/err_education.go +++ b/services/graph/pkg/identity/err_education.go @@ -29,6 +29,11 @@ func (i *ErrEducationBackend) GetEducationSchools(ctx context.Context) ([]*libre return nil, errNotImplemented } +// FilterEducationSchoolsByAttribute implements the EducationBackend interface for the ErrEducationBackend backend. +func (i *ErrEducationBackend) FilterEducationSchoolsByAttribute(ctx context.Context, attr, value string) ([]*libregraph.EducationSchool, error) { + return nil, errNotImplemented +} + // UpdateEducationSchool implements the EducationBackend interface for the ErrEducationBackend backend. func (i *ErrEducationBackend) UpdateEducationSchool(ctx context.Context, numberOrID string, school libregraph.EducationSchool) (*libregraph.EducationSchool, error) { return nil, errNotImplemented diff --git a/services/graph/pkg/identity/ldap_education_school.go b/services/graph/pkg/identity/ldap_education_school.go index 2aecc3d9bb..68f2b4225b 100644 --- a/services/graph/pkg/identity/ldap_education_school.go +++ b/services/graph/pkg/identity/ldap_education_school.go @@ -49,10 +49,9 @@ const ( ) var ( - errNotSet = errors.New("attribute not set") - errSchoolNameExists = errorcode.New(errorcode.NameAlreadyExists, "A school with that name is already present") - errSchoolNumberExists = errorcode.New(errorcode.NameAlreadyExists, "A school with that number is already present") - errSchoolExternalIdExists = errorcode.New(errorcode.NameAlreadyExists, "A school with that external id is already present") + errNotSet = errors.New("attribute not set") + errSchoolNameExists = errorcode.New(errorcode.NameAlreadyExists, "A school with that name is already present") + errSchoolNumberExists = errorcode.New(errorcode.NameAlreadyExists, "A school with that number is already present") ) func defaultEducationConfig() educationConfig { @@ -136,21 +135,6 @@ func (i *LDAP) CreateEducationSchool(ctx context.Context, school libregraph.Educ } } - // Check that the school external id is not already used - if school.HasExternalId() { - _, err := i.getSchoolByExternalId(school.GetExternalId()) - switch { - case err == nil: - logger.Debug().Err(errSchoolExternalIdExists).Str("externalId", school.GetExternalId()).Msg("duplicate school external id") - return nil, errSchoolExternalIdExists - case errors.Is(err, ErrNotFound): - break - default: - logger.Error().Err(err).Str("externalId", school.GetExternalId()).Msg("error looking up school by external id") - return nil, errorcode.New(errorcode.GeneralException, "error looking up school by external id") - } - } - attributeTypeAndValue := ldap.AttributeTypeAndValue{ Type: i.educationConfig.schoolAttributeMap.displayName, Value: school.GetDisplayName(), @@ -299,14 +283,14 @@ func (i *LDAP) updateSchoolProperties(ctx context.Context, dn string, currentSch } // UpdateEducationSchool updates the supplied school in the identity backend -func (i *LDAP) UpdateEducationSchool(ctx context.Context, numberOrIDOrExternalID string, school libregraph.EducationSchool) (*libregraph.EducationSchool, error) { +func (i *LDAP) UpdateEducationSchool(ctx context.Context, numberOrID string, school libregraph.EducationSchool) (*libregraph.EducationSchool, error) { logger := i.logger.SubloggerWithRequestID(ctx) logger.Debug().Str("backend", "ldap").Msg("UpdateEducationSchool") if !i.writeEnabled { return nil, ErrReadOnly } - e, err := i.getSchoolByNumberOrIDOrExternalID(numberOrIDOrExternalID) + e, err := i.getSchoolByNumberOrID(numberOrID) if err != nil { return nil, err } @@ -329,7 +313,7 @@ func (i *LDAP) UpdateEducationSchool(ctx context.Context, numberOrIDOrExternalID } // Read back school from LDAP - e, err = i.getSchoolByNumberOrIDOrExternalID(i.getID(e)) + e, err = i.getSchoolByNumberOrID(i.getID(e)) if err != nil { return nil, err } @@ -343,7 +327,7 @@ func (i *LDAP) DeleteEducationSchool(ctx context.Context, id string) error { if !i.writeEnabled { return ErrReadOnly } - e, err := i.getSchoolByNumberOrIDOrExternalID(id) + e, err := i.getSchoolByNumberOrID(id) if err != nil { return err } @@ -358,10 +342,10 @@ func (i *LDAP) DeleteEducationSchool(ctx context.Context, id string) error { } // GetEducationSchool implements the EducationBackend interface for the LDAP backend. -func (i *LDAP) GetEducationSchool(ctx context.Context, numberOrIDOrExternalID string) (*libregraph.EducationSchool, error) { +func (i *LDAP) GetEducationSchool(ctx context.Context, numberOrID string) (*libregraph.EducationSchool, error) { logger := i.logger.SubloggerWithRequestID(ctx) logger.Debug().Str("backend", "ldap").Msg("GetEducationSchool") - e, err := i.getSchoolByNumberOrIDOrExternalID(numberOrIDOrExternalID) + e, err := i.getSchoolByNumberOrID(numberOrID) if err != nil { return nil, err } @@ -410,6 +394,56 @@ func (i *LDAP) GetEducationSchools(ctx context.Context) ([]*libregraph.Education return schools, nil } +// FilterEducationSchoolsByAttribute implements the EducationBackend interface for the LDAP backend. +func (i *LDAP) FilterEducationSchoolsByAttribute(ctx context.Context, attr, value string) ([]*libregraph.EducationSchool, error) { + logger := i.logger.SubloggerWithRequestID(ctx).With().Str("func", "FilterEducationSchoolsByAttribute").Logger() + logger.Debug().Str("backend", "ldap").Str("attribute", attr).Str("value", value).Msg("") + + var ldapAttr string + switch attr { + case "externalId": + ldapAttr = i.educationConfig.schoolAttributeMap.externalId + default: + return nil, errorcode.New(errorcode.InvalidRequest, fmt.Sprintf("filtering by attribute '%s' is not supported", attr)) + } + filter := fmt.Sprintf("(&%s(objectClass=%s)(%s=%s))", + i.educationConfig.schoolFilter, + i.educationConfig.schoolObjectClass, + ldap.EscapeFilter(ldapAttr), + ldap.EscapeFilter(value), + ) + + searchRequest := ldap.NewSearchRequest( + i.educationConfig.schoolBaseDN, + i.educationConfig.schoolScope, + ldap.NeverDerefAliases, 0, 0, false, + filter, + i.getEducationSchoolAttrTypes(), + nil, + ) + logger.Debug().Str("base", searchRequest.BaseDN). + Str("filter", searchRequest.Filter). + Int("scope", searchRequest.Scope). + Int("sizelimit", searchRequest.SizeLimit). + Interface("attributes", searchRequest.Attributes). + Msg("LDAP Search Request") + + res, err := i.conn.Search(searchRequest) + if err != nil { + return nil, errorcode.New(errorcode.ItemNotFound, err.Error()) + } + + schools := make([]*libregraph.EducationSchool, 0, len(res.Entries)) + for _, e := range res.Entries { + school := i.createSchoolModelFromLDAP(e) + if school == nil { + continue + } + schools = append(schools, school) + } + return schools, nil +} + // GetEducationSchoolUsers implements the EducationBackend interface for the LDAP backend. func (i *LDAP) GetEducationSchoolUsers(ctx context.Context, schoolNumberOrID string) ([]*libregraph.EducationUser, error) { logger := i.logger.SubloggerWithRequestID(ctx) @@ -436,11 +470,11 @@ func (i *LDAP) GetEducationSchoolUsers(ctx context.Context, schoolNumberOrID str } // AddUsersToEducationSchool adds new members (reference by a slice of IDs) to supplied school in the identity backend. -func (i *LDAP) AddUsersToEducationSchool(ctx context.Context, schoolNumberOrIDOrExternalID string, memberIDs []string) error { +func (i *LDAP) AddUsersToEducationSchool(ctx context.Context, schoolNumberOrID string, memberIDs []string) error { logger := i.logger.SubloggerWithRequestID(ctx) logger.Debug().Str("backend", "ldap").Msg("AddUsersToEducationSchool") - schoolEntry, err := i.getSchoolByNumberOrIDOrExternalID(schoolNumberOrIDOrExternalID) + schoolEntry, err := i.getSchoolByNumberOrID(schoolNumberOrID) if err != nil { return err } @@ -483,11 +517,11 @@ func (i *LDAP) AddUsersToEducationSchool(ctx context.Context, schoolNumberOrIDOr } // RemoveUserFromEducationSchool removes a single member (by ID) from a school -func (i *LDAP) RemoveUserFromEducationSchool(ctx context.Context, schoolNumberOrIDOrExternalID string, memberID string) error { +func (i *LDAP) RemoveUserFromEducationSchool(ctx context.Context, schoolNumberOrID string, memberID string) error { logger := i.logger.SubloggerWithRequestID(ctx) logger.Debug().Str("backend", "ldap").Msg("RemoveUserFromEducationSchool") - schoolEntry, err := i.getSchoolByNumberOrIDOrExternalID(schoolNumberOrIDOrExternalID) + schoolEntry, err := i.getSchoolByNumberOrID(schoolNumberOrID) if err != nil { return err } @@ -542,12 +576,12 @@ func (i *LDAP) GetEducationSchoolClasses(ctx context.Context, schoolNumberOrID s } func (i *LDAP) getEducationSchoolEntries( - schoolNumberOrIDOrExternalID, filter, objectClass, baseDN string, + schoolNumberOrID, filter, objectClass, baseDN string, scope int, attributes []string, logger log.Logger, ) ([]*ldap.Entry, error) { - schoolEntry, err := i.getSchoolByNumberOrIDOrExternalID(schoolNumberOrIDOrExternalID) + schoolEntry, err := i.getSchoolByNumberOrID(schoolNumberOrID) if err != nil { return nil, err } @@ -584,11 +618,11 @@ func (i *LDAP) getEducationSchoolEntries( } // AddClassesToEducationSchool adds new members (reference by a slice of IDs) to supplied school in the identity backend. -func (i *LDAP) AddClassesToEducationSchool(ctx context.Context, schoolNumberOrIDOrExternalID string, memberIDs []string) error { +func (i *LDAP) AddClassesToEducationSchool(ctx context.Context, schoolNumberOrID string, memberIDs []string) error { logger := i.logger.SubloggerWithRequestID(ctx) logger.Debug().Str("backend", "ldap").Msg("AddClassesToEducationSchool") - schoolEntry, err := i.getSchoolByNumberOrIDOrExternalID(schoolNumberOrIDOrExternalID) + schoolEntry, err := i.getSchoolByNumberOrID(schoolNumberOrID) if err != nil { return err } @@ -631,11 +665,11 @@ func (i *LDAP) AddClassesToEducationSchool(ctx context.Context, schoolNumberOrID } // RemoveClassFromEducationSchool removes a single member (by ID) from a school -func (i *LDAP) RemoveClassFromEducationSchool(ctx context.Context, schoolNumberOrIDOrExternalID string, memberID string) error { +func (i *LDAP) RemoveClassFromEducationSchool(ctx context.Context, schoolNumberOrID string, memberID string) error { logger := i.logger.SubloggerWithRequestID(ctx) logger.Debug().Str("backend", "ldap").Msg("RemoveClassFromEducationSchool") - schoolEntry, err := i.getSchoolByNumberOrIDOrExternalID(schoolNumberOrIDOrExternalID) + schoolEntry, err := i.getSchoolByNumberOrID(schoolNumberOrID) if err != nil { return err } @@ -673,16 +707,14 @@ func (i *LDAP) getSchoolByDN(dn string) (*ldap.Entry, error) { return i.getEntryByDN(dn, i.getEducationSchoolAttrTypes(), filter) } -func (i *LDAP) getSchoolByNumberOrIDOrExternalID(numberOrIDOrExternalID string) (*ldap.Entry, error) { - numberOrIDOrExternalID = ldap.EscapeFilter(numberOrIDOrExternalID) +func (i *LDAP) getSchoolByNumberOrID(numberOrID string) (*ldap.Entry, error) { + numberOrID = ldap.EscapeFilter(numberOrID) filter := fmt.Sprintf( - "(|(%s=%s)(%s=%s)(%s=%s))", + "(|(%s=%s)(%s=%s))", i.educationConfig.schoolAttributeMap.id, - numberOrIDOrExternalID, + numberOrID, i.educationConfig.schoolAttributeMap.schoolNumber, - numberOrIDOrExternalID, - i.educationConfig.schoolAttributeMap.externalId, - numberOrIDOrExternalID, + numberOrID, ) return i.getSchoolByFilter(filter) } @@ -697,16 +729,6 @@ func (i *LDAP) getSchoolByNumber(schoolNumber string) (*ldap.Entry, error) { return i.getSchoolByFilter(filter) } -func (i *LDAP) getSchoolByExternalId(schoolExternalId string) (*ldap.Entry, error) { - schoolExternalId = ldap.EscapeFilter(schoolExternalId) - filter := fmt.Sprintf( - "(%s=%s)", - i.educationConfig.schoolAttributeMap.externalId, - schoolExternalId, - ) - return i.getSchoolByFilter(filter) -} - func (i *LDAP) getSchoolByFilter(filter string) (*ldap.Entry, error) { filter = fmt.Sprintf("(&%s(objectClass=%s)%s)", i.educationConfig.schoolFilter, @@ -820,6 +842,7 @@ func (i *LDAP) getEducationSchoolAttrTypes() []string { return []string{ i.educationConfig.schoolAttributeMap.displayName, i.educationConfig.schoolAttributeMap.id, + i.educationConfig.schoolAttributeMap.externalId, i.educationConfig.schoolAttributeMap.schoolNumber, i.educationConfig.schoolAttributeMap.terminationDate, } diff --git a/services/graph/pkg/identity/ldap_education_school_test.go b/services/graph/pkg/identity/ldap_education_school_test.go index 5bc7718184..b47f088d6b 100644 --- a/services/graph/pkg/identity/ldap_education_school_test.go +++ b/services/graph/pkg/identity/ldap_education_school_test.go @@ -65,10 +65,10 @@ var schoolEntryWithTermination = ldap.NewEntry("ou=Test School", }) var ( - filterSchoolSearchByIdExisting = "(&(objectClass=openCloudEducationSchool)(|(openCloudUUID=abcd-defg)(openCloudEducationSchoolNumber=abcd-defg)(openCloudEducationExternalId=abcd-defg)))" - filterSchoolSearchByIdNonexistant = "(&(objectClass=openCloudEducationSchool)(|(openCloudUUID=xxxx-xxxx)(openCloudEducationSchoolNumber=xxxx-xxxx)(openCloudEducationExternalId=xxxx-xxxx)))" - filterSchoolSearchByNumberExisting = "(&(objectClass=openCloudEducationSchool)(|(openCloudUUID=0123)(openCloudEducationSchoolNumber=0123)(openCloudEducationExternalId=0123)))" - filterSchoolSearchByNumberNonexistant = "(&(objectClass=openCloudEducationSchool)(|(openCloudUUID=3210)(openCloudEducationSchoolNumber=3210)(openCloudEducationExternalId=3210)))" + filterSchoolSearchByIdExisting = "(&(objectClass=openCloudEducationSchool)(|(openCloudUUID=abcd-defg)(openCloudEducationSchoolNumber=abcd-defg)))" + filterSchoolSearchByIdNonexistant = "(&(objectClass=openCloudEducationSchool)(|(openCloudUUID=xxxx-xxxx)(openCloudEducationSchoolNumber=xxxx-xxxx)))" + filterSchoolSearchByNumberExisting = "(&(objectClass=openCloudEducationSchool)(|(openCloudUUID=0123)(openCloudEducationSchoolNumber=0123)))" + filterSchoolSearchByNumberNonexistant = "(&(objectClass=openCloudEducationSchool)(|(openCloudUUID=3210)(openCloudEducationSchoolNumber=3210)))" ) func TestCreateEducationSchool(t *testing.T) { @@ -128,7 +128,7 @@ func TestCreateEducationSchool(t *testing.T) { Scope: 2, SizeLimit: 1, Filter: "(&(objectClass=openCloudEducationSchool)(openCloudEducationSchoolNumber=0123))", - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, Controls: []ldap.Control(nil), } lm.On("Search", schoolNumberSearchRequest). @@ -142,7 +142,7 @@ func TestCreateEducationSchool(t *testing.T) { Scope: 2, SizeLimit: 1, Filter: "(&(objectClass=openCloudEducationSchool)(openCloudEducationSchoolNumber=0666))", - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, Controls: []ldap.Control(nil), } lm.On("Search", existingSchoolNumberSearchRequest). @@ -156,7 +156,7 @@ func TestCreateEducationSchool(t *testing.T) { Scope: 2, SizeLimit: 1, Filter: "(&(objectClass=openCloudEducationSchool)(openCloudEducationSchoolNumber=1111))", - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, Controls: []ldap.Control(nil), } lm.On("Search", schoolNumberSearchRequestError). @@ -170,7 +170,7 @@ func TestCreateEducationSchool(t *testing.T) { Scope: 0, SizeLimit: 1, Filter: "(objectClass=openCloudEducationSchool)", - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, Controls: []ldap.Control(nil), } lm.On("Search", schoolLookupAfterCreate). @@ -358,7 +358,7 @@ func TestDeleteEducationSchool(t *testing.T) { Scope: 2, SizeLimit: 1, Filter: tt.filter, - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, Controls: []ldap.Control(nil), } if tt.expectedItemNotFound { @@ -427,7 +427,7 @@ func TestGetEducationSchool(t *testing.T) { Scope: 2, SizeLimit: 1, Filter: tt.filter, - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, Controls: []ldap.Control(nil), } if tt.expectedItemNotFound { @@ -461,7 +461,7 @@ func TestGetEducationSchools(t *testing.T) { Scope: 2, SizeLimit: 0, Filter: "(objectClass=openCloudEducationSchool)", - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, Controls: []ldap.Control(nil), } lm.On("Search", sr1).Return(&ldap.SearchResult{Entries: []*ldap.Entry{schoolEntry, schoolEntry1}}, nil) @@ -478,7 +478,7 @@ var schoolByIDSearch1 *ldap.SearchRequest = &ldap.SearchRequest{ Scope: 2, SizeLimit: 1, Filter: filterSchoolSearchByIdExisting, - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, Controls: []ldap.Control(nil), } @@ -487,7 +487,7 @@ var schoolByNumberSearch *ldap.SearchRequest = &ldap.SearchRequest{ Scope: 2, SizeLimit: 1, Filter: filterSchoolSearchByNumberExisting, - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, Controls: []ldap.Control(nil), } diff --git a/services/graph/pkg/identity/mocks/education_backend.go b/services/graph/pkg/identity/mocks/education_backend.go index 8933df7104..f710fa9467 100644 --- a/services/graph/pkg/identity/mocks/education_backend.go +++ b/services/graph/pkg/identity/mocks/education_backend.go @@ -602,6 +602,80 @@ func (_c *EducationBackend_DeleteEducationUser_Call) RunAndReturn(run func(ctx c return _c } +// FilterEducationSchoolsByAttribute provides a mock function for the type EducationBackend +func (_mock *EducationBackend) FilterEducationSchoolsByAttribute(ctx context.Context, attr string, value string) ([]*libregraph.EducationSchool, error) { + ret := _mock.Called(ctx, attr, value) + + if len(ret) == 0 { + panic("no return value specified for FilterEducationSchoolsByAttribute") + } + + var r0 []*libregraph.EducationSchool + var r1 error + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) ([]*libregraph.EducationSchool, error)); ok { + return returnFunc(ctx, attr, value) + } + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) []*libregraph.EducationSchool); ok { + r0 = returnFunc(ctx, attr, value) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*libregraph.EducationSchool) + } + } + if returnFunc, ok := ret.Get(1).(func(context.Context, string, string) error); ok { + r1 = returnFunc(ctx, attr, value) + } else { + r1 = ret.Error(1) + } + return r0, r1 +} + +// EducationBackend_FilterEducationSchoolsByAttribute_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'FilterEducationSchoolsByAttribute' +type EducationBackend_FilterEducationSchoolsByAttribute_Call struct { + *mock.Call +} + +// FilterEducationSchoolsByAttribute is a helper method to define mock.On call +// - ctx context.Context +// - attr string +// - value string +func (_e *EducationBackend_Expecter) FilterEducationSchoolsByAttribute(ctx interface{}, attr interface{}, value interface{}) *EducationBackend_FilterEducationSchoolsByAttribute_Call { + return &EducationBackend_FilterEducationSchoolsByAttribute_Call{Call: _e.mock.On("FilterEducationSchoolsByAttribute", ctx, attr, value)} +} + +func (_c *EducationBackend_FilterEducationSchoolsByAttribute_Call) Run(run func(ctx context.Context, attr string, value string)) *EducationBackend_FilterEducationSchoolsByAttribute_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 context.Context + if args[0] != nil { + arg0 = args[0].(context.Context) + } + var arg1 string + if args[1] != nil { + arg1 = args[1].(string) + } + var arg2 string + if args[2] != nil { + arg2 = args[2].(string) + } + run( + arg0, + arg1, + arg2, + ) + }) + return _c +} + +func (_c *EducationBackend_FilterEducationSchoolsByAttribute_Call) Return(educationSchools []*libregraph.EducationSchool, err error) *EducationBackend_FilterEducationSchoolsByAttribute_Call { + _c.Call.Return(educationSchools, err) + return _c +} + +func (_c *EducationBackend_FilterEducationSchoolsByAttribute_Call) RunAndReturn(run func(ctx context.Context, attr string, value string) ([]*libregraph.EducationSchool, error)) *EducationBackend_FilterEducationSchoolsByAttribute_Call { + _c.Call.Return(run) + return _c +} + // FilterEducationUsersByAttribute provides a mock function for the type EducationBackend func (_mock *EducationBackend) FilterEducationUsersByAttribute(ctx context.Context, attr string, value string) ([]*libregraph.EducationUser, error) { ret := _mock.Called(ctx, attr, value) diff --git a/services/graph/pkg/service/v0/educationschools.go b/services/graph/pkg/service/v0/educationschools.go index 6e1a8756b2..119783c418 100644 --- a/services/graph/pkg/service/v0/educationschools.go +++ b/services/graph/pkg/service/v0/educationschools.go @@ -30,11 +30,36 @@ func (g Graph) GetEducationSchools(w http.ResponseWriter, r *http.Request) { return } - schools, err := g.identityEducationBackend.GetEducationSchools(r.Context()) - if err != nil { - logger.Debug().Err(err).Msg("could not get schools: backend error") - errorcode.RenderError(w, r, err) - return + var schools []*libregraph.EducationSchool + if odataReq.Query.Filter != nil { + attr, value, err := g.getEqualityFilter(r.Context(), odataReq) + if err != nil { + logger.Debug().Err(err).Str("filter", odataReq.Query.Filter.RawValue).Msg("failed to parse filter") + var errcode errorcode.Error + var godataerr *godata.GoDataError + switch { + case errors.As(err, &errcode): + errcode.Render(w, r) + case errors.As(err, &godataerr): + errorcode.GeneralException.Render(w, r, godataerr.ResponseCode, err.Error()) + default: + errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error()) + } + return + } + schools, err = g.identityEducationBackend.FilterEducationSchoolsByAttribute(r.Context(), attr, value) + if err != nil { + logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get schools from backend") + errorcode.RenderError(w, r, err) + return + } + } else { + schools, err = g.identityEducationBackend.GetEducationSchools(r.Context()) + if err != nil { + logger.Debug().Err(err).Msg("could not get schools: backend error") + errorcode.RenderError(w, r, err) + return + } } schools, err = sortEducationSchools(odataReq, schools) diff --git a/services/graph/pkg/service/v0/educationschools_test.go b/services/graph/pkg/service/v0/educationschools_test.go index c9ede03947..79f4c2357a 100644 --- a/services/graph/pkg/service/v0/educationschools_test.go +++ b/services/graph/pkg/service/v0/educationschools_test.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "net/http/httptest" + "net/url" "time" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" @@ -176,6 +177,18 @@ var _ = Describe("Schools", func() { Expect(len(res.Value)).To(Equal(1)) Expect(res.Value[0].GetId()).To(Equal("school1")) }) + + When("used with a filter", func() { + It("fails with an unsupported filter", func() { + svc.GetEducationSchools(rr, httptest.NewRequest(http.MethodGet, "/graph/v1.0/education/schools?$filter="+url.QueryEscape("displayName ne 'test'"), nil)) + Expect(rr.Code).To(Equal(http.StatusNotImplemented)) + }) + It("calls the backend with the externalId filter", func() { + identityEducationBackend.On("FilterEducationSchoolsByAttribute", mock.Anything, "externalId", "ext1234").Return([]*libregraph.EducationSchool{newSchool}, nil) + svc.GetEducationSchools(rr, httptest.NewRequest(http.MethodGet, "/graph/v1.0/education/schools?$filter="+url.QueryEscape("externalId eq 'ext1234'"), nil)) + Expect(rr.Code).To(Equal(http.StatusOK)) + }) + }) }) Describe("GetEducationSchool", func() { From 9f7b42586b41873f90cc587ad746269017db45f9 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 4 Mar 2026 14:36:00 +0100 Subject: [PATCH 4/5] chore(graph/education): reduce complexity and duplication --- .../pkg/identity/ldap_education_school.go | 44 +++++-------- .../identity/ldap_education_school_test.go | 20 +++--- .../graph/pkg/service/v0/educationschools.go | 48 ++++++-------- .../graph/pkg/service/v0/educationuser.go | 62 +++++++++---------- 4 files changed, 76 insertions(+), 98 deletions(-) diff --git a/services/graph/pkg/identity/ldap_education_school.go b/services/graph/pkg/identity/ldap_education_school.go index 68f2b4225b..9c171fc0e5 100644 --- a/services/graph/pkg/identity/ldap_education_school.go +++ b/services/graph/pkg/identity/ldap_education_school.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "slices" "time" "github.com/go-ldap/ldap/v3" @@ -496,26 +497,25 @@ func (i *LDAP) AddUsersToEducationSchool(ctx context.Context, schoolNumberOrID s } for _, userEntry := range userEntries { - currentSchools := userEntry.GetEqualFoldAttributeValues(i.educationConfig.memberOfSchoolAttribute) - found := false - for _, currentSchool := range currentSchools { - if currentSchool == schoolID { - found = true - break - } - } - if !found { - mr := ldap.ModifyRequest{DN: userEntry.DN} - mr.Add(i.educationConfig.memberOfSchoolAttribute, []string{schoolID}) - if err := i.conn.Modify(&mr); err != nil { - return err - } + if err := i.addEntryToSchool(userEntry, schoolID); err != nil { + return err } } return nil } +// addEntryToSchool adds the schoolID to the entry's memberOfSchool attribute if not already present. +func (i *LDAP) addEntryToSchool(entry *ldap.Entry, schoolID string) error { + currentSchools := entry.GetEqualFoldAttributeValues(i.educationConfig.memberOfSchoolAttribute) + if slices.Contains(currentSchools, schoolID) { + return nil + } + mr := ldap.ModifyRequest{DN: entry.DN} + mr.Add(i.educationConfig.memberOfSchoolAttribute, []string{schoolID}) + return i.conn.Modify(&mr) +} + // RemoveUserFromEducationSchool removes a single member (by ID) from a school func (i *LDAP) RemoveUserFromEducationSchool(ctx context.Context, schoolNumberOrID string, memberID string) error { logger := i.logger.SubloggerWithRequestID(ctx) @@ -644,20 +644,8 @@ func (i *LDAP) AddClassesToEducationSchool(ctx context.Context, schoolNumberOrID } for _, classEntry := range classEntries { - currentSchools := classEntry.GetEqualFoldAttributeValues(i.educationConfig.memberOfSchoolAttribute) - found := false - for _, currentSchool := range currentSchools { - if currentSchool == schoolID { - found = true - break - } - } - if !found { - mr := ldap.ModifyRequest{DN: classEntry.DN} - mr.Add(i.educationConfig.memberOfSchoolAttribute, []string{schoolID}) - if err := i.conn.Modify(&mr); err != nil { - return err - } + if err := i.addEntryToSchool(classEntry, schoolID); err != nil { + return err } } diff --git a/services/graph/pkg/identity/ldap_education_school_test.go b/services/graph/pkg/identity/ldap_education_school_test.go index b47f088d6b..0eb4fcc048 100644 --- a/services/graph/pkg/identity/ldap_education_school_test.go +++ b/services/graph/pkg/identity/ldap_education_school_test.go @@ -69,6 +69,8 @@ var ( filterSchoolSearchByIdNonexistant = "(&(objectClass=openCloudEducationSchool)(|(openCloudUUID=xxxx-xxxx)(openCloudEducationSchoolNumber=xxxx-xxxx)))" filterSchoolSearchByNumberExisting = "(&(objectClass=openCloudEducationSchool)(|(openCloudUUID=0123)(openCloudEducationSchoolNumber=0123)))" filterSchoolSearchByNumberNonexistant = "(&(objectClass=openCloudEducationSchool)(|(openCloudUUID=3210)(openCloudEducationSchoolNumber=3210)))" + + schoolLDAPAttributeTypes = []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"} ) func TestCreateEducationSchool(t *testing.T) { @@ -128,7 +130,7 @@ func TestCreateEducationSchool(t *testing.T) { Scope: 2, SizeLimit: 1, Filter: "(&(objectClass=openCloudEducationSchool)(openCloudEducationSchoolNumber=0123))", - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: schoolLDAPAttributeTypes, Controls: []ldap.Control(nil), } lm.On("Search", schoolNumberSearchRequest). @@ -142,7 +144,7 @@ func TestCreateEducationSchool(t *testing.T) { Scope: 2, SizeLimit: 1, Filter: "(&(objectClass=openCloudEducationSchool)(openCloudEducationSchoolNumber=0666))", - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: schoolLDAPAttributeTypes, Controls: []ldap.Control(nil), } lm.On("Search", existingSchoolNumberSearchRequest). @@ -156,7 +158,7 @@ func TestCreateEducationSchool(t *testing.T) { Scope: 2, SizeLimit: 1, Filter: "(&(objectClass=openCloudEducationSchool)(openCloudEducationSchoolNumber=1111))", - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: schoolLDAPAttributeTypes, Controls: []ldap.Control(nil), } lm.On("Search", schoolNumberSearchRequestError). @@ -170,7 +172,7 @@ func TestCreateEducationSchool(t *testing.T) { Scope: 0, SizeLimit: 1, Filter: "(objectClass=openCloudEducationSchool)", - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: schoolLDAPAttributeTypes, Controls: []ldap.Control(nil), } lm.On("Search", schoolLookupAfterCreate). @@ -358,7 +360,7 @@ func TestDeleteEducationSchool(t *testing.T) { Scope: 2, SizeLimit: 1, Filter: tt.filter, - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: schoolLDAPAttributeTypes, Controls: []ldap.Control(nil), } if tt.expectedItemNotFound { @@ -427,7 +429,7 @@ func TestGetEducationSchool(t *testing.T) { Scope: 2, SizeLimit: 1, Filter: tt.filter, - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: schoolLDAPAttributeTypes, Controls: []ldap.Control(nil), } if tt.expectedItemNotFound { @@ -461,7 +463,7 @@ func TestGetEducationSchools(t *testing.T) { Scope: 2, SizeLimit: 0, Filter: "(objectClass=openCloudEducationSchool)", - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: schoolLDAPAttributeTypes, Controls: []ldap.Control(nil), } lm.On("Search", sr1).Return(&ldap.SearchResult{Entries: []*ldap.Entry{schoolEntry, schoolEntry1}}, nil) @@ -478,7 +480,7 @@ var schoolByIDSearch1 *ldap.SearchRequest = &ldap.SearchRequest{ Scope: 2, SizeLimit: 1, Filter: filterSchoolSearchByIdExisting, - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: schoolLDAPAttributeTypes, Controls: []ldap.Control(nil), } @@ -487,7 +489,7 @@ var schoolByNumberSearch *ldap.SearchRequest = &ldap.SearchRequest{ Scope: 2, SizeLimit: 1, Filter: filterSchoolSearchByNumberExisting, - Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}, + Attributes: schoolLDAPAttributeTypes, Controls: []ldap.Control(nil), } diff --git a/services/graph/pkg/service/v0/educationschools.go b/services/graph/pkg/service/v0/educationschools.go index 119783c418..d363a43494 100644 --- a/services/graph/pkg/service/v0/educationschools.go +++ b/services/graph/pkg/service/v0/educationschools.go @@ -1,6 +1,7 @@ package svc import ( + "context" "errors" "fmt" "net/http" @@ -30,36 +31,11 @@ func (g Graph) GetEducationSchools(w http.ResponseWriter, r *http.Request) { return } - var schools []*libregraph.EducationSchool - if odataReq.Query.Filter != nil { - attr, value, err := g.getEqualityFilter(r.Context(), odataReq) - if err != nil { - logger.Debug().Err(err).Str("filter", odataReq.Query.Filter.RawValue).Msg("failed to parse filter") - var errcode errorcode.Error - var godataerr *godata.GoDataError - switch { - case errors.As(err, &errcode): - errcode.Render(w, r) - case errors.As(err, &godataerr): - errorcode.GeneralException.Render(w, r, godataerr.ResponseCode, err.Error()) - default: - errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error()) - } - return - } - schools, err = g.identityEducationBackend.FilterEducationSchoolsByAttribute(r.Context(), attr, value) - if err != nil { - logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get schools from backend") - errorcode.RenderError(w, r, err) - return - } - } else { - schools, err = g.identityEducationBackend.GetEducationSchools(r.Context()) - if err != nil { - logger.Debug().Err(err).Msg("could not get schools: backend error") - errorcode.RenderError(w, r, err) - return - } + schools, err := g.getEducationSchoolsFromBackend(r.Context(), odataReq) + if err != nil { + logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get schools") + renderEqualityFilterError(w, r, err) + return } schools, err = sortEducationSchools(odataReq, schools) @@ -641,3 +617,15 @@ func sortEducationSchools(req *godata.GoDataRequest, schools []*libregraph.Educa return schools, nil } + +// getEducationSchoolsFromBackend fetches schools from the backend, applying an OData $filter if present. +func (g Graph) getEducationSchoolsFromBackend(ctx context.Context, odataReq *godata.GoDataRequest) ([]*libregraph.EducationSchool, error) { + if odataReq.Query.Filter != nil { + attr, value, err := g.getEqualityFilter(ctx, odataReq) + if err != nil { + return nil, err + } + return g.identityEducationBackend.FilterEducationSchoolsByAttribute(ctx, attr, value) + } + return g.identityEducationBackend.GetEducationSchools(ctx) +} diff --git a/services/graph/pkg/service/v0/educationuser.go b/services/graph/pkg/service/v0/educationuser.go index 8fa87c5eec..389eccf665 100644 --- a/services/graph/pkg/service/v0/educationuser.go +++ b/services/graph/pkg/service/v0/educationuser.go @@ -32,38 +32,12 @@ func (g Graph) GetEducationUsers(w http.ResponseWriter, r *http.Request) { return } - var users []*libregraph.EducationUser - if odataReq.Query.Filter != nil { - attr, value, err := g.getEqualityFilter(r.Context(), odataReq) - if err != nil { - logger.Debug().Err(err).Str("filter", odataReq.Query.Filter.RawValue).Msg("failed to parse filter") - var errcode errorcode.Error - var godataerr *godata.GoDataError - switch { - case errors.As(err, &errcode): - errcode.Render(w, r) - case errors.As(err, &godataerr): - errorcode.GeneralException.Render(w, r, godataerr.ResponseCode, err.Error()) - default: - errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error()) - } - return - } - users, err = g.identityEducationBackend.FilterEducationUsersByAttribute(r.Context(), attr, value) - if err != nil { - logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get education users from backend") - errorcode.RenderError(w, r, err) - return - } - } else { - users, err = g.identityEducationBackend.GetEducationUsers(r.Context()) - if err != nil { - logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get education users from backend") - errorcode.RenderError(w, r, err) - return - } + users, err := g.getEducationUsersFromBackend(r.Context(), odataReq) + if err != nil { + logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get education users") + renderEqualityFilterError(w, r, err) + return } - users, err = sortEducationUsers(odataReq, users) if err != nil { logger.Debug().Interface("query", odataReq).Msg("error while sorting education users according to query") @@ -413,6 +387,18 @@ func sortEducationUsers(req *godata.GoDataRequest, users []*libregraph.Education return users, nil } +// getEducationUsersFromBackend fetches users from the backend, applying an OData $filter if present. +func (g Graph) getEducationUsersFromBackend(ctx context.Context, odataReq *godata.GoDataRequest) ([]*libregraph.EducationUser, error) { + if odataReq.Query.Filter != nil { + attr, value, err := g.getEqualityFilter(ctx, odataReq) + if err != nil { + return nil, err + } + return g.identityEducationBackend.FilterEducationUsersByAttribute(ctx, attr, value) + } + return g.identityEducationBackend.GetEducationUsers(ctx) +} + func (g Graph) getEqualityFilter(ctx context.Context, req *godata.GoDataRequest) (string, string, error) { logger := g.logger.SubloggerWithRequestID(ctx) @@ -439,3 +425,17 @@ func (g Graph) getEqualityFilter(ctx context.Context, req *godata.GoDataRequest) value := strings.Trim(root.Children[1].Token.Value, "'") return root.Children[0].Token.Value, value, nil } + +// renderEqualityFilterError writes the appropriate HTTP error response for errors returned by getEqualityFilter. +func renderEqualityFilterError(w http.ResponseWriter, r *http.Request, err error) { + var errcode errorcode.Error + var godataerr *godata.GoDataError + switch { + case errors.As(err, &errcode): + errcode.Render(w, r) + case errors.As(err, &godataerr): + errorcode.GeneralException.Render(w, r, godataerr.ResponseCode, err.Error()) + default: + errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error()) + } +} From fe3befd172bf624bd8c028c0f9cdb60a492284b6 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 4 Mar 2026 15:36:22 +0100 Subject: [PATCH 5/5] chore(graph/education): deduplicate LDAP Search result processing --- .../pkg/identity/ldap_education_school.go | 47 ++++------------ .../graph/pkg/identity/ldap_education_user.go | 54 +++++-------------- 2 files changed, 23 insertions(+), 78 deletions(-) diff --git a/services/graph/pkg/identity/ldap_education_school.go b/services/graph/pkg/identity/ldap_education_school.go index 9c171fc0e5..68ee855b61 100644 --- a/services/graph/pkg/identity/ldap_education_school.go +++ b/services/graph/pkg/identity/ldap_education_school.go @@ -356,43 +356,11 @@ func (i *LDAP) GetEducationSchool(ctx context.Context, numberOrID string) (*libr // GetEducationSchools implements the EducationBackend interface for the LDAP backend. func (i *LDAP) GetEducationSchools(ctx context.Context) ([]*libregraph.EducationSchool, error) { - var filter string - filter = fmt.Sprintf("(objectClass=%s)", i.educationConfig.schoolObjectClass) - + filter := fmt.Sprintf("(objectClass=%s)", i.educationConfig.schoolObjectClass) if i.educationConfig.schoolFilter != "" { filter = fmt.Sprintf("(&%s%s)", i.educationConfig.schoolFilter, filter) } - - searchRequest := ldap.NewSearchRequest( - i.educationConfig.schoolBaseDN, - i.educationConfig.schoolScope, - ldap.NeverDerefAliases, 0, 0, false, - filter, - i.getEducationSchoolAttrTypes(), - nil, - ) - i.logger.Debug().Str("backend", "ldap"). - Str("base", searchRequest.BaseDN). - Str("filter", searchRequest.Filter). - Int("scope", searchRequest.Scope). - Int("sizelimit", searchRequest.SizeLimit). - Interface("attributes", searchRequest.Attributes). - Msg("GetEducationSchools") - res, err := i.conn.Search(searchRequest) - if err != nil { - return nil, errorcode.New(errorcode.ItemNotFound, err.Error()) - } - - schools := make([]*libregraph.EducationSchool, 0, len(res.Entries)) - for _, e := range res.Entries { - school := i.createSchoolModelFromLDAP(e) - // Skip invalid LDAP entries - if school == nil { - continue - } - schools = append(schools, school) - } - return schools, nil + return i.searchEducationSchools(ctx, filter) } // FilterEducationSchoolsByAttribute implements the EducationBackend interface for the LDAP backend. @@ -413,7 +381,11 @@ func (i *LDAP) FilterEducationSchoolsByAttribute(ctx context.Context, attr, valu ldap.EscapeFilter(ldapAttr), ldap.EscapeFilter(value), ) + return i.searchEducationSchools(ctx, filter) +} +// searchEducationSchools builds and executes an LDAP search for education schools and converts the results to EducationSchool models. +func (i *LDAP) searchEducationSchools(ctx context.Context, filter string) ([]*libregraph.EducationSchool, error) { searchRequest := ldap.NewSearchRequest( i.educationConfig.schoolBaseDN, i.educationConfig.schoolScope, @@ -422,12 +394,14 @@ func (i *LDAP) FilterEducationSchoolsByAttribute(ctx context.Context, attr, valu i.getEducationSchoolAttrTypes(), nil, ) - logger.Debug().Str("base", searchRequest.BaseDN). + logger := i.logger.SubloggerWithRequestID(ctx) + logger.Debug().Str("backend", "ldap"). + Str("base", searchRequest.BaseDN). Str("filter", searchRequest.Filter). Int("scope", searchRequest.Scope). Int("sizelimit", searchRequest.SizeLimit). Interface("attributes", searchRequest.Attributes). - Msg("LDAP Search Request") + Msg("searchEducationSchools") res, err := i.conn.Search(searchRequest) if err != nil { @@ -437,6 +411,7 @@ func (i *LDAP) FilterEducationSchoolsByAttribute(ctx context.Context, attr, valu schools := make([]*libregraph.EducationSchool, 0, len(res.Entries)) for _, e := range res.Entries { school := i.createSchoolModelFromLDAP(e) + // Skip invalid LDAP entries if school == nil { continue } diff --git a/services/graph/pkg/identity/ldap_education_user.go b/services/graph/pkg/identity/ldap_education_user.go index 5a5bf0bb66..88855d52e0 100644 --- a/services/graph/pkg/identity/ldap_education_user.go +++ b/services/graph/pkg/identity/ldap_education_user.go @@ -208,48 +208,13 @@ func (i *LDAP) GetEducationUser(ctx context.Context, nameOrID string) (*libregra // GetEducationUsers implements the EducationBackend interface for the LDAP backend. func (i *LDAP) GetEducationUsers(ctx context.Context) ([]*libregraph.EducationUser, error) { - logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("GetEducationUsers") - - var userFilter string - + var filter string if i.userFilter == "" { - userFilter = fmt.Sprintf("(objectClass=%s)", i.educationConfig.userObjectClass) + filter = fmt.Sprintf("(objectClass=%s)", i.educationConfig.userObjectClass) } else { - userFilter = fmt.Sprintf("(&%s(objectClass=%s))", i.userFilter, i.educationConfig.userObjectClass) - } - - searchRequest := ldap.NewSearchRequest( - i.userBaseDN, - i.userScope, - ldap.NeverDerefAliases, 0, 0, false, - userFilter, - i.getEducationUserAttrTypes(), - nil, - ) - logger.Debug().Str("backend", "ldap"). - Str("base", searchRequest.BaseDN). - Str("filter", searchRequest.Filter). - Int("scope", searchRequest.Scope). - Int("sizelimit", searchRequest.SizeLimit). - Interface("attributes", searchRequest.Attributes). - Msg("GetEducationUsers") - res, err := i.conn.Search(searchRequest) - if err != nil { - return nil, errorcode.New(errorcode.ItemNotFound, err.Error()) + filter = fmt.Sprintf("(&%s(objectClass=%s))", i.userFilter, i.educationConfig.userObjectClass) } - - users := make([]*libregraph.EducationUser, 0, len(res.Entries)) - - for _, e := range res.Entries { - u := i.createEducationUserModelFromLDAP(e) - // Skip invalid LDAP users - if u == nil { - continue - } - users = append(users, u) - } - return users, nil + return i.searchEducationUsers(ctx, filter) } func (i *LDAP) FilterEducationUsersByAttribute(ctx context.Context, attr, value string) ([]*libregraph.EducationUser, error) { @@ -272,7 +237,11 @@ func (i *LDAP) FilterEducationUsersByAttribute(ctx context.Context, attr, value return nil, errorcode.New(errorcode.InvalidRequest, fmt.Sprintf("filtering by attribute '%s' is not supported", attr)) } filter := fmt.Sprintf("(&%s(objectClass=%s)(%s=%s))", i.userFilter, i.educationConfig.userObjectClass, ldap.EscapeFilter(ldapAttr), ldap.EscapeFilter(value)) + return i.searchEducationUsers(ctx, filter) +} +// searchEducationUsers builds and executes an LDAP search for education users and converts the results to EducationUser models. +func (i *LDAP) searchEducationUsers(ctx context.Context, filter string) ([]*libregraph.EducationUser, error) { searchRequest := ldap.NewSearchRequest( i.userBaseDN, i.userScope, @@ -281,12 +250,14 @@ func (i *LDAP) FilterEducationUsersByAttribute(ctx context.Context, attr, value i.getEducationUserAttrTypes(), nil, ) - logger.Debug().Str("base", searchRequest.BaseDN). + logger := i.logger.SubloggerWithRequestID(ctx) + logger.Debug().Str("backend", "ldap"). + Str("base", searchRequest.BaseDN). Str("filter", searchRequest.Filter). Int("scope", searchRequest.Scope). Int("sizelimit", searchRequest.SizeLimit). Interface("attributes", searchRequest.Attributes). - Msg("LDAP Search Request") + Msg("searchEducationUsers") res, err := i.conn.Search(searchRequest) if err != nil { @@ -294,7 +265,6 @@ func (i *LDAP) FilterEducationUsersByAttribute(ctx context.Context, attr, value } users := make([]*libregraph.EducationUser, 0, len(res.Entries)) - for _, e := range res.Entries { u := i.createEducationUserModelFromLDAP(e) // Skip invalid LDAP users