From 9f7b42586b41873f90cc587ad746269017db45f9 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 4 Mar 2026 14:36:00 +0100 Subject: [PATCH] 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()) + } +}