Adding so education PATCH updates school name or number.

services/graph/pkg/identity/backend.go

@@ -51,6 +51,8 @@ type EducationBackend interface {
 	GetEducationSchool(ctx context.Context, nameOrID string, queryParam url.Values) (*libregraph.EducationSchool, error)
 	// GetEducationSchools lists all schools
 	GetEducationSchools(ctx context.Context, queryParam url.Values) ([]*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
 	GetEducationSchoolUsers(ctx context.Context, id string) ([]*libregraph.EducationUser, error)
 	// AddUsersToEducationSchool adds new members (reference by a slice of IDs) to supplied school in the identity backend.

services/graph/pkg/identity/err_school.go

@@ -30,6 +30,11 @@ func (i *ErrEducationBackend) GetEducationSchools(ctx context.Context, queryPara
 	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
+}
+
 // GetEducationSchoolUsers implements the EducationBackend interface for the ErrEducationBackend backend.
 func (i *ErrEducationBackend) GetEducationSchoolUsers(ctx context.Context, id string) ([]*libregraph.EducationUser, error) {
 	return nil, errNotImplemented

services/graph/pkg/identity/ldap_school.go

@@ -34,6 +34,15 @@ type schoolAttributeMap struct {
 	id           string
 }
 
+type SchoolUpdateOperation uint8
+
+const (
+	TooManyValues SchoolUpdateOperation = iota
+	SchoolUnchanged
+	DisplayNameUpdated
+	SchoolNumberUpdated
+)
+
 func defaultEducationConfig() educationConfig {
 	return educationConfig{
 		schoolObjectClass:       "ocEducationSchool",
@@ -95,6 +104,8 @@ func (i *LDAP) CreateEducationSchool(ctx context.Context, school libregraph.Educ
 		return nil, ErrReadOnly
 	}
 
+	// FIXME: Verify that the school number is not already in use
+
 	dn := fmt.Sprintf("%s=%s,%s",
 		i.educationConfig.schoolAttributeMap.displayName,
 		oldap.EscapeDNAttributeValue(school.GetDisplayName()),
@@ -128,6 +139,130 @@ func (i *LDAP) CreateEducationSchool(ctx context.Context, school libregraph.Educ
 	return i.createSchoolModelFromLDAP(e), nil
 }
 
+// UpdateEducationSchoolOperation contains the logic for which update operation to apply to a school
+func (i *LDAP) UpdateEducationSchoolOperation(
+	schoolUpdate libregraph.EducationSchool,
+	currentSchool libregraph.EducationSchool,
+) SchoolUpdateOperation {
+	providedDisplayName := schoolUpdate.GetDisplayName()
+	schoolNumber := schoolUpdate.GetSchoolNumber()
+
+	if providedDisplayName != "" && schoolNumber != "" {
+		return TooManyValues
+	}
+
+	if providedDisplayName != "" && providedDisplayName != currentSchool.GetDisplayName() {
+		return DisplayNameUpdated
+	}
+
+	if schoolNumber != "" && schoolNumber != currentSchool.GetSchoolNumber() {
+		return SchoolNumberUpdated
+	}
+
+	return SchoolUnchanged
+}
+
+// updateDisplayName updates the school OU in the identity backend
+func (i *LDAP) updateDisplayName(ctx context.Context, dn string, providedDisplayName string) error {
+	logger := i.logger.SubloggerWithRequestID(ctx)
+	newDisplayName := fmt.Sprintf(
+		"%s=%s",
+		i.educationConfig.schoolAttributeMap.displayName,
+		providedDisplayName,
+	)
+
+	mrdn := ldap.NewModifyDNRequest(dn, newDisplayName, true, "")
+	i.logger.Debug().Str("backend", "ldap").
+		Str("dn", mrdn.DN).
+		Str("newrdn", mrdn.NewRDN).
+		Msg("updateDisplayName")
+
+	if err := i.conn.ModifyDN(mrdn); err != nil {
+		var lerr *ldap.Error
+		logger.Debug().Err(err).Msg("error updating school name")
+		if errors.As(err, &lerr) {
+			if lerr.ResultCode == ldap.LDAPResultEntryAlreadyExists {
+				err = errorcode.New(errorcode.NameAlreadyExists, lerr.Error())
+			}
+		}
+		return err
+	}
+
+	return nil
+}
+
+// updateSchoolNumber checks if a school number is already taken, and if not updates the school number
+func (i *LDAP) updateSchoolNumber(ctx context.Context, dn string, schoolNumber string) error {
+	logger := i.logger.SubloggerWithRequestID(ctx)
+	_, err := i.getSchoolByNumberOrID(schoolNumber)
+	if err == nil {
+		errmsg := fmt.Sprintf("school number '%s' already exists", schoolNumber)
+		err = fmt.Errorf(errmsg)
+		return err
+	}
+
+	mr := ldap.NewModifyRequest(dn, nil)
+	mr.Replace(i.educationConfig.schoolAttributeMap.schoolNumber, []string{schoolNumber})
+
+	if err := i.conn.Modify(mr); err != nil {
+		var lerr *ldap.Error
+		logger.Debug().Err(err).Msg("error updating school number")
+		if errors.As(err, &lerr) {
+			if lerr.ResultCode == ldap.LDAPResultEntryAlreadyExists {
+				err = errorcode.New(errorcode.NameAlreadyExists, lerr.Error())
+			}
+		}
+		return err
+	}
+
+	return nil
+}
+
+// UpdateEducationSchool updates the supplied school in the identity backend
+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
+	}
+
+	providedDisplayName := school.GetDisplayName()
+	schoolNumber := school.GetSchoolNumber()
+
+	if providedDisplayName != "" && schoolNumber != "" {
+		return nil, fmt.Errorf("school name and school number cannot be updated in the same request")
+	}
+
+	e, err := i.getSchoolByNumberOrID(numberOrID)
+	if err != nil {
+		return nil, err
+	}
+
+	currentSchool := i.createSchoolModelFromLDAP(e)
+	switch i.UpdateEducationSchoolOperation(school, *currentSchool) {
+	case TooManyValues:
+		return nil, fmt.Errorf("school name and school number cannot be updated in the same request")
+	case SchoolUnchanged:
+		logger.Debug().Str("backend", "ldap").Msg("UpdateEducationSchool: Nothing changed")
+		return i.createSchoolModelFromLDAP(e), nil
+	case DisplayNameUpdated:
+		if err := i.updateDisplayName(ctx, e.DN, providedDisplayName); err != nil {
+			return nil, err
+		}
+	case SchoolNumberUpdated:
+		if err := i.updateSchoolNumber(ctx, e.DN, schoolNumber); err != nil {
+			return nil, err
+		}
+	}
+
+	// Read	back school from LDAP
+	e, err = i.getSchoolByNumberOrID(i.getID(e))
+	if err != nil {
+		return nil, err
+	}
+	return i.createSchoolModelFromLDAP(e), nil
+}
+
 // DeleteEducationSchool deletes a given school, identified by id
 func (i *LDAP) DeleteEducationSchool(ctx context.Context, id string) error {
 	logger := i.logger.SubloggerWithRequestID(ctx)
@@ -408,9 +543,9 @@ func (i *LDAP) createSchoolModelFromLDAP(e *ldap.Entry) *libregraph.EducationSch
 		return nil
 	}
 
-	displayName := e.GetEqualFoldAttributeValue(i.educationConfig.schoolAttributeMap.displayName)
+	displayName := i.getDisplayName(e)
-	id := e.GetEqualFoldAttributeValue(i.educationConfig.schoolAttributeMap.id)
+	id := i.getID(e)
-	schoolNumber := e.GetEqualFoldAttributeValue(i.educationConfig.schoolAttributeMap.schoolNumber)
+	schoolNumber := i.getSchoolNumber(e)
 
 	if id != "" && displayName != "" && schoolNumber != "" {
 		school := libregraph.NewEducationSchool()
@@ -422,3 +557,18 @@ func (i *LDAP) createSchoolModelFromLDAP(e *ldap.Entry) *libregraph.EducationSch
 	i.logger.Warn().Str("dn", e.DN).Str("id", id).Str("displayName", displayName).Str("schoolNumber", schoolNumber).Msg("Invalid School. Missing required attribute")
 	return nil
 }
+
+func (i *LDAP) getSchoolNumber(e *ldap.Entry) string {
+	schoolNumber := e.GetEqualFoldAttributeValue(i.educationConfig.schoolAttributeMap.schoolNumber)
+	return schoolNumber
+}
+
+func (i *LDAP) getID(e *ldap.Entry) string {
+	id := e.GetEqualFoldAttributeValue(i.educationConfig.schoolAttributeMap.id)
+	return id
+}
+
+func (i *LDAP) getDisplayName(e *ldap.Entry) string {
+	displayName := e.GetEqualFoldAttributeValue(i.educationConfig.schoolAttributeMap.displayName)
+	return displayName
+}

services/graph/pkg/identity/ldap_school_test.go

@@ -46,6 +46,11 @@ var schoolEntry1 = ldap.NewEntry("ou=Test School1",
 		"owncloudUUID":            {"hijk-defg"},
 	})
 
+var filterSchoolSearchByIdExisting = "(&(objectClass=ocEducationSchool)(|(owncloudUUID=abcd-defg)(ocEducationSchoolNumber=abcd-defg)))"
+var filterSchoolSearchByIdNonexistant = "(&(objectClass=ocEducationSchool)(|(owncloudUUID=xxxx-xxxx)(ocEducationSchoolNumber=xxxx-xxxx)))"
+var filterSchoolSearchByNumberExisting = "(&(objectClass=ocEducationSchool)(|(owncloudUUID=0123)(ocEducationSchoolNumber=0123)))"
+var filterSchoolSearchByNumberNonexistant = "(&(objectClass=ocEducationSchool)(|(owncloudUUID=3210)(ocEducationSchoolNumber=3210)))"
+
 func TestCreateEducationSchool(t *testing.T) {
 	lm := &mocks.Client{}
 	lm.On("Add", mock.Anything).
@@ -75,6 +80,64 @@ func TestCreateEducationSchool(t *testing.T) {
 	assert.Equal(t, res_school.GetSchoolNumber(), school.GetSchoolNumber())
 }
 
+func TestUpdateEducationSchoolOperation(t *testing.T) {
+	tests := []struct {
+		name              string
+		displayName       string
+		schoolNumber      string
+		expectedOperation SchoolUpdateOperation
+	}{
+		{
+			name:              "Test using school with both number and name",
+			displayName:       "A name",
+			schoolNumber:      "1234",
+			expectedOperation: TooManyValues,
+		},
+		{
+			name:              "Test with unchanged number",
+			schoolNumber:      "1234",
+			expectedOperation: SchoolUnchanged,
+		},
+		{
+			name:              "Test with unchanged name",
+			displayName:       "A name",
+			expectedOperation: SchoolUnchanged,
+		},
+		{
+			name:              "Test new name",
+			displayName:       "Something new",
+			expectedOperation: DisplayNameUpdated,
+		},
+		{
+			name:              "Test new number",
+			schoolNumber:      "9876",
+			expectedOperation: SchoolNumberUpdated,
+		},
+	}
+
+	for _, tt := range tests {
+		lm := &mocks.Client{}
+		b, err := getMockedBackend(lm, eduConfig, &logger)
+		assert.Nil(t, err)
+
+		displayName := "A name"
+		schoolNumber := "1234"
+
+		currentSchool := libregraph.EducationSchool{
+			DisplayName:  &displayName,
+			SchoolNumber: &schoolNumber,
+		}
+
+		schoolUpdate := libregraph.EducationSchool{
+			DisplayName:  &tt.displayName,
+			SchoolNumber: &tt.schoolNumber,
+		}
+
+		operation := b.UpdateEducationSchoolOperation(schoolUpdate, currentSchool)
+		assert.Equal(t, tt.expectedOperation, operation)
+	}
+}
+
 func TestDeleteEducationSchool(t *testing.T) {
 	tests := []struct {
 		name                 string
@@ -85,25 +148,25 @@ func TestDeleteEducationSchool(t *testing.T) {
 		{
 			name:                 "Test delete school using schoolId",
 			numberOrId:           "abcd-defg",
-			filter:               "(&(objectClass=ocEducationSchool)(|(owncloudUUID=abcd-defg)(ocEducationSchoolNumber=abcd-defg)))",
+			filter:               filterSchoolSearchByIdExisting,
 			expectedItemNotFound: false,
 		},
 		{
 			name:                 "Test delete school using unknown schoolId",
 			numberOrId:           "xxxx-xxxx",
-			filter:               "(&(objectClass=ocEducationSchool)(|(owncloudUUID=xxxx-xxxx)(ocEducationSchoolNumber=xxxx-xxxx)))",
+			filter:               filterSchoolSearchByIdNonexistant,
 			expectedItemNotFound: true,
 		},
 		{
 			name:                 "Test delete school using schoolNumber",
 			numberOrId:           "0123",
-			filter:               "(&(objectClass=ocEducationSchool)(|(owncloudUUID=0123)(ocEducationSchoolNumber=0123)))",
+			filter:               filterSchoolSearchByNumberExisting,
 			expectedItemNotFound: false,
 		},
 		{
 			name:                 "Test delete school using unknown schoolNumber",
 			numberOrId:           "3210",
-			filter:               "(&(objectClass=ocEducationSchool)(|(owncloudUUID=3210)(ocEducationSchoolNumber=3210)))",
+			filter:               filterSchoolSearchByNumberNonexistant,
 			expectedItemNotFound: true,
 		},
 	}
@@ -154,25 +217,25 @@ func TestGetEducationSchool(t *testing.T) {
 		{
 			name:                 "Test search school using schoolId",
 			numberOrId:           "abcd-defg",
-			filter:               "(&(objectClass=ocEducationSchool)(|(owncloudUUID=abcd-defg)(ocEducationSchoolNumber=abcd-defg)))",
+			filter:               filterSchoolSearchByIdExisting,
 			expectedItemNotFound: false,
 		},
 		{
 			name:                 "Test search school using unknown schoolId",
 			numberOrId:           "xxxx-xxxx",
-			filter:               "(&(objectClass=ocEducationSchool)(|(owncloudUUID=xxxx-xxxx)(ocEducationSchoolNumber=xxxx-xxxx)))",
+			filter:               filterSchoolSearchByIdNonexistant,
 			expectedItemNotFound: true,
 		},
 		{
 			name:                 "Test search school using schoolNumber",
 			numberOrId:           "0123",
-			filter:               "(&(objectClass=ocEducationSchool)(|(owncloudUUID=0123)(ocEducationSchoolNumber=0123)))",
+			filter:               filterSchoolSearchByNumberExisting,
 			expectedItemNotFound: false,
 		},
 		{
 			name:                 "Test search school using unknown schoolNumber",
 			numberOrId:           "3210",
-			filter:               "(&(objectClass=ocEducationSchool)(|(owncloudUUID=3210)(ocEducationSchoolNumber=3210)))",
+			filter:               filterSchoolSearchByNumberNonexistant,
 			expectedItemNotFound: true,
 		},
 	}
@@ -234,7 +297,7 @@ var schoolByIDSearch1 *ldap.SearchRequest = &ldap.SearchRequest{
 	BaseDN:     "",
 	Scope:      2,
 	SizeLimit:  1,
-	Filter:     "(&(objectClass=ocEducationSchool)(|(owncloudUUID=abcd-defg)(ocEducationSchoolNumber=abcd-defg)))",
+	Filter:     filterSchoolSearchByIdExisting,
 	Attributes: []string{"ou", "owncloudUUID", "ocEducationSchoolNumber"},
 	Controls:   []ldap.Control(nil),
 }

services/graph/pkg/identity/mocks/education_backend.go

@@ -234,6 +234,29 @@ func (_m *EducationBackend) RemoveUserFromEducationSchool(ctx context.Context, s
 	return r0
 }
 
+// UpdateEducationSchool provides a mock function with given fields: ctx, numberOrID, school
+func (_m *EducationBackend) UpdateEducationSchool(ctx context.Context, numberOrID string, school libregraph.EducationSchool) (*libregraph.EducationSchool, error) {
+	ret := _m.Called(ctx, numberOrID, school)
+
+	var r0 *libregraph.EducationSchool
+	if rf, ok := ret.Get(0).(func(context.Context, string, libregraph.EducationSchool) *libregraph.EducationSchool); ok {
+		r0 = rf(ctx, numberOrID, school)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(*libregraph.EducationSchool)
+		}
+	}
+
+	var r1 error
+	if rf, ok := ret.Get(1).(func(context.Context, string, libregraph.EducationSchool) error); ok {
+		r1 = rf(ctx, numberOrID, school)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
 // UpdateEducationUser provides a mock function with given fields: ctx, nameOrID, user
 func (_m *EducationBackend) UpdateEducationUser(ctx context.Context, nameOrID string, user libregraph.EducationUser) (*libregraph.EducationUser, error) {
 	ret := _m.Called(ctx, nameOrID, user)

services/graph/pkg/service/v0/educationschools.go

@@ -112,24 +112,31 @@ func (g Graph) PatchEducationSchool(w http.ResponseWriter, r *http.Request) {
 	schoolID := chi.URLParam(r, "schoolID")
 	schoolID, err := url.PathUnescape(schoolID)
 	if err != nil {
-		logger.Debug().Str("id", schoolID).Msg("could not change school: unescaping school id failed")
+		logger.Debug().Str("id", schoolID).Msg("could not update school: unescaping school id failed")
 		errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "unescaping school id failed")
 		return
 	}
 
 	if schoolID == "" {
-		logger.Debug().Msg("could not change school: missing school id")
+		logger.Debug().Msg("could not update school: missing school id")
 		errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "missing school id")
 		return
 	}
-	changes := libregraph.NewEducationSchool()
+
-	err = json.NewDecoder(r.Body).Decode(changes)
+	school := libregraph.NewEducationSchool()
+	err = json.NewDecoder(r.Body).Decode(school)
 	if err != nil {
-		logger.Debug().Err(err).Interface("body", r.Body).Msg("could not change school: invalid request body")
+		logger.Debug().Err(err).Interface("body", r.Body).Msg("could not update school: invalid request body")
 		errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, fmt.Sprintf("invalid request body: %s", err.Error()))
 		return
 	}
 
+	if school, err = g.identityEducationBackend.UpdateEducationSchool(r.Context(), schoolID, *school); err != nil {
+		logger.Debug().Err(err).Interface("school", school).Msg("could not update school: backend error")
+		errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
+		return
+	}
+
 	/* TODO requires reva changes
 	e := events.SchoolFeatureChanged{SchoolID: schoolID}
 	if currentUser, ok := ctxpkg.ContextGetUser(r.Context()); ok {
@@ -138,8 +145,8 @@ func (g Graph) PatchEducationSchool(w http.ResponseWriter, r *http.Request) {
 	g.publishEvent(e)
 	*/
 
-	render.Status(r, http.StatusNoContent)
+	render.Status(r, http.StatusOK)
-	render.NoContent(w, r)
+	render.JSON(w, r, school)
 }
 
 // GetEducationSchool implements the Service interface.

services/graph/pkg/service/v0/educationschools_test.go

@@ -315,6 +315,8 @@ var _ = Describe("Schools", func() {
 			newSchoolJson, err := json.Marshal(newSchool)
 			Expect(err).ToNot(HaveOccurred())
 
+			identityEducationBackend.On("UpdateEducationSchool", mock.Anything, mock.Anything, mock.Anything).Return(newSchool, nil)
+
 			r := httptest.NewRequest(http.MethodPatch, "/graph/v1.0/education/schools/schoolid", bytes.NewBuffer(newSchoolJson))
 			rctx := chi.NewRouteContext()
 			rctx.URLParams.Add("schoolID", "school-id")
@@ -322,7 +324,7 @@ var _ = Describe("Schools", func() {
 
 			svc.PatchEducationSchool(rr, r)
 
-			Expect(rr.Code).To(Equal(http.StatusNoContent))
+			Expect(rr.Code).To(Equal(http.StatusOK))
 		})
 	})