From 28ec9c3282a0ac2939bd89fb84ed5a3444402546 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Tue, 14 Oct 2025 16:52:23 +0200 Subject: [PATCH] graph(education): Make 'schoolNumber' attribute optional It's already optional in the spec. For mulit-tenant provisioning we want it to be optional as well. Related: #1597 --- .../pkg/identity/ldap_education_school.go | 54 +++++++++++-------- .../graph/pkg/service/v0/educationschools.go | 6 --- .../pkg/service/v0/educationschools_test.go | 12 ----- 3 files changed, 31 insertions(+), 41 deletions(-) diff --git a/services/graph/pkg/identity/ldap_education_school.go b/services/graph/pkg/identity/ldap_education_school.go index 71941b6898..efd916e699 100644 --- a/services/graph/pkg/identity/ldap_education_school.go +++ b/services/graph/pkg/identity/ldap_education_school.go @@ -8,10 +8,10 @@ import ( "github.com/go-ldap/ldap/v3" "github.com/gofrs/uuid" + libregraph "github.com/opencloud-eu/libre-graph-api-go" "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/services/graph/pkg/config" "github.com/opencloud-eu/opencloud/services/graph/pkg/errorcode" - libregraph "github.com/opencloud-eu/libre-graph-api-go" ) type educationConfig struct { @@ -119,16 +119,18 @@ func (i *LDAP) CreateEducationSchool(ctx context.Context, school libregraph.Educ } // Check that the school number is not already used - _, err := i.getSchoolByNumber(school.GetSchoolNumber()) - switch err { - case nil: - logger.Debug().Err(errSchoolNumberExists).Str("schoolNumber", school.GetSchoolNumber()).Msg("duplicate school number") - return nil, errSchoolNumberExists - case ErrNotFound: - break - default: - logger.Error().Err(err).Str("schoolNumber", school.GetSchoolNumber()).Msg("error looking up school by number") - return nil, errorcode.New(errorcode.GeneralException, "error looking up school by number") + if school.HasSchoolNumber() { + _, err := i.getSchoolByNumber(school.GetSchoolNumber()) + switch err { + case nil: + logger.Debug().Err(errSchoolNumberExists).Str("schoolNumber", school.GetSchoolNumber()).Msg("duplicate school number") + return nil, errSchoolNumberExists + case ErrNotFound: + break + default: + logger.Error().Err(err).Str("schoolNumber", school.GetSchoolNumber()).Msg("error looking up school by number") + return nil, errorcode.New(errorcode.GeneralException, "error looking up school by number") + } } attributeTypeAndValue := ldap.AttributeTypeAndValue{ @@ -142,7 +144,9 @@ func (i *LDAP) CreateEducationSchool(ctx context.Context, school libregraph.Educ ) ar := ldap.NewAddRequest(dn, nil) ar.Attribute(i.educationConfig.schoolAttributeMap.displayName, []string{school.GetDisplayName()}) - ar.Attribute(i.educationConfig.schoolAttributeMap.schoolNumber, []string{school.GetSchoolNumber()}) + if school.HasSchoolNumber() { + ar.Attribute(i.educationConfig.schoolAttributeMap.schoolNumber, []string{school.GetSchoolNumber()}) + } if !i.useServerUUID { ar.Attribute(i.educationConfig.schoolAttributeMap.id, []string{uuid.Must(uuid.NewV4()).String()}) } @@ -723,18 +727,22 @@ func (i *LDAP) createSchoolModelFromLDAP(e *ldap.Entry) *libregraph.EducationSch if err != nil && !errors.Is(err, errNotSet) { i.logger.Error().Err(err).Str("dn", e.DN).Msg("Error reading termination date for LDAP entry") } - if id != "" && displayName != "" && schoolNumber != "" { - school := libregraph.NewEducationSchool() - school.SetDisplayName(displayName) - school.SetSchoolNumber(schoolNumber) - school.SetId(id) - if t != nil { - school.SetTerminationDate(*t) - } - return school + + if id == "" || displayName == "" { + i.logger.Warn().Str("dn", e.DN).Str("id", id).Str("displayName", displayName).Msg("Invalid School. Missing required attribute") + return nil } - i.logger.Warn().Str("dn", e.DN).Str("id", id).Str("displayName", displayName).Str("schoolNumber", schoolNumber).Msg("Invalid School. Missing required attribute") - return nil + + school := libregraph.NewEducationSchool() + school.SetDisplayName(displayName) + school.SetId(id) + if schoolNumber != "" { + school.SetSchoolNumber(schoolNumber) + } + if t != nil { + school.SetTerminationDate(*t) + } + return school } func (i *LDAP) getSchoolNumber(e *ldap.Entry) string { diff --git a/services/graph/pkg/service/v0/educationschools.go b/services/graph/pkg/service/v0/educationschools.go index 3888cfac8f..6e1a8756b2 100644 --- a/services/graph/pkg/service/v0/educationschools.go +++ b/services/graph/pkg/service/v0/educationschools.go @@ -74,12 +74,6 @@ func (g Graph) PostEducationSchool(w http.ResponseWriter, r *http.Request) { return } - if _, ok := school.GetSchoolNumberOk(); !ok { - logger.Debug().Interface("school", school).Msg("could not create school: missing required attribute") - errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "Missing Required Attribute") - return - } - // validate terminationDate attribute, needs to be "far enough" in the future, terminationDate can be nil (means // termination date is to be deleted if terminationDate, ok := school.GetTerminationDateOk(); ok && terminationDate != nil { diff --git a/services/graph/pkg/service/v0/educationschools_test.go b/services/graph/pkg/service/v0/educationschools_test.go index 7c4311ebd8..c9ede03947 100644 --- a/services/graph/pkg/service/v0/educationschools_test.go +++ b/services/graph/pkg/service/v0/educationschools_test.go @@ -232,18 +232,6 @@ var _ = Describe("Schools", func() { Expect(rr.Code).To(Equal(http.StatusBadRequest)) }) - It("handles missing school number", func() { - newSchool = libregraph.NewEducationSchool() - newSchool.SetDisplayName("New School") - newSchoolJson, err := json.Marshal(newSchool) - Expect(err).ToNot(HaveOccurred()) - - r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/education/schools/", bytes.NewBuffer(newSchoolJson)) - - svc.PostEducationSchool(rr, r) - Expect(rr.Code).To(Equal(http.StatusBadRequest)) - }) - It("disallows school create ids", func() { newSchool = libregraph.NewEducationSchool() newSchool.SetId("disallowed")