Repository: owncloud/ocis (go)

Issue:
libregraph: DELETEing a school should delete all relations
When a school is deprovisioned, all relations to users and classes should be deleted as well.

> Note: Users must not be deleted when they are no longer part of a school. They have to be deleted with explicit requests against the `/users/{id | onpremisessamaccountname}` or `/education/users/{id | onpremisessamaccountname}`

Pull request:
Disassociate users from schools on school delete.

<!--
Thanks for submitting a change to ownCloud!

This is the bug tracker for the oCIS component. Find other components at https://github.com/owncloud/core/blob/master/.github/CONTRIBUTING.md#guidelines

For fixing potential security issues please see https://owncloud.org/security/

To make it possible for us to get your change reviewed and merged please carefully fill out the requested information below.

Please note that any kind of change needs first be submitted to the master branch which holds the next version of oCIS.

Please set the following labels:

- Set label "Status:Needs-Review" for review or "Status:In-Progress" if the PR still has open tasks.
- Assignment: assign to self
- Reviewers: pick at least one
-->

## Description
This PR alters the `DeleteEducationSchool` to also disassociate the
users that were associated with the to-be-deleted school.


## Related Issue
<!--- This project only accepts pull requests related to open issues -->
<!--- If suggesting a new feature or change, please discuss it in an issue first -->
<!--- If fixing a bug, there should be an issue describing it with steps to reproduce -->
<!--- Please link to the issue here: -->
- Fixes #5246 

## Motivation and Context
It solves that users still point to a non-existent school.

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->
- test environment: I tested it with a local ldap, removing a school and checking if users were disassociated.
- ...

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] Technical debt
- [ ] Tests only (no source changes)

## Checklist:
<!-- Tick the checkboxes when done. -->
<!-- Raise documentation ticket in owncloud.github.io/ -->
- [x] Code changes
- [x] Unit tests added
- [ ] Acceptance tests added
- [ ] Documentation ticket raised: <link> 


Patch:
diff --git a/changelog/unreleased/bugfix-disassociate-users-from-deleted-school.md b/changelog/unreleased/bugfix-disassociate-users-from-deleted-school.md
new file mode 100644
index 00000000000..59cd2f900d1
--- /dev/null
+++ b/changelog/unreleased/bugfix-disassociate-users-from-deleted-school.md
@@ -0,0 +1,6 @@
+Bugfix: Disassociate users from deleted school
+
+When a school is deleted, users should be disassociated from it.
+
+https://github.com/owncloud/ocis/pull/5343
+https://github.com/owncloud/ocis/issues/5246
diff --git a/services/graph/pkg/service/v0/educationschools.go b/services/graph/pkg/service/v0/educationschools.go
index 08c51dbca21..b3e9fcb20e6 100644
--- a/services/graph/pkg/service/v0/educationschools.go
+++ b/services/graph/pkg/service/v0/educationschools.go
@@ -11,6 +11,7 @@ import (
 
 	"github.com/CiscoM31/godata"
 	libregraph "github.com/owncloud/libre-graph-api-go"
+	"github.com/owncloud/ocis/v2/services/graph/pkg/identity"
 	"github.com/owncloud/ocis/v2/services/graph/pkg/service/v0/errorcode"
 
 	"github.com/go-chi/chi/v5"
@@ -195,17 +196,32 @@ func (g Graph) DeleteEducationSchool(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	logger.Debug().Str("schoolID", schoolID).Msg("Getting users of school")
+	users, err := g.identityEducationBackend.GetEducationSchoolUsers(r.Context(), schoolID)
+	if err != nil {
+		logger.Debug().Err(err).Msg("could not get school users: backend error")
+		renderInternalServerError(w, r, err)
+		return
+	}
+
+	for _, user := range users {
+		logger.Debug().Str("schoolID", schoolID).Str("userID", *user.Id).Msg("calling delete member on backend")
+		if err := g.identityEducationBackend.RemoveUserFromEducationSchool(r.Context(), schoolID, *user.Id); err != nil {
+			if errors.Is(err, identity.ErrNotFound) {
+				logger.Debug().Str("schoolID", schoolID).Str("userID", *user.Id).Msg("user not found")
+				continue
+			}
+			logger.Debug().Err(err).Msg("could not delete school member: backend error")
+			renderInternalServerError(w, r, err)
+		}
+	}
+
 	logger.Debug().Str("id", schoolID).Msg("calling delete school on backend")
 	err = g.identityEducationBackend.DeleteEducationSchool(r.Context(), schoolID)
 
 	if err != nil {
 		logger.Debug().Err(err).Msg("could not delete school: backend error")
-		var errcode errorcode.Error
-		if errors.As(err, &errcode) {
-			errcode.Render(w, r)
-		} else {
-			errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
-		}
+		renderInternalServerError(w, r, err)
 		return
 	}
 
@@ -221,6 +237,15 @@ func (g Graph) DeleteEducationSchool(w http.ResponseWriter, r *http.Request) {
 	render.NoContent(w, r)
 }
 
+func renderInternalServerError(w http.ResponseWriter, r *http.Request, err error) {
+	var errcode errorcode.Error
+	if errors.As(err, &errcode) {
+		errcode.Render(w, r)
+	} else {
+		errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
+	}
+}
+
 // GetEducationSchoolUsers implements the Service interface.
 func (g Graph) GetEducationSchoolUsers(w http.ResponseWriter, r *http.Request) {
 	logger := g.logger.SubloggerWithRequestID(r.Context())


Test patch:
diff --git a/services/graph/pkg/service/v0/educationschools_test.go b/services/graph/pkg/service/v0/educationschools_test.go
index 52c2404778a..b94878cc394 100644
--- a/services/graph/pkg/service/v0/educationschools_test.go
+++ b/services/graph/pkg/service/v0/educationschools_test.go
@@ -50,7 +50,6 @@ var _ = Describe("Schools", func() {
 	)
 
 	BeforeEach(func() {
-
 		identityEducationBackend = &identitymocks.EducationBackend{}
 		gatewayClient = &mocks.GatewayClient{}
 		newSchool = libregraph.NewEducationSchool()
@@ -336,6 +335,27 @@ var _ = Describe("Schools", func() {
 
 		It("deletes the school", func() {
 			identityEducationBackend.On("DeleteEducationSchool", mock.Anything, mock.Anything, mock.Anything).Return(nil)
+			identityEducationBackend.On("GetEducationSchoolUsers", mock.Anything, mock.Anything, mock.Anything).Return([]*libregraph.EducationUser{}, nil)
+			r := httptest.NewRequest(http.MethodPatch, "/graph/v1.0/education/schools", nil)
+			rctx := chi.NewRouteContext()
+			rctx.URLParams.Add("schoolID", *newSchool.Id)
+			r = r.WithContext(context.WithValue(ctxpkg.ContextSetUser(ctx, currentUser), chi.RouteCtxKey, rctx))
+			svc.DeleteEducationSchool(rr, r)
+
+			Expect(rr.Code).To(Equal(http.StatusNoContent))
+			identityEducationBackend.AssertNumberOfCalls(GinkgoT(), "DeleteEducationSchool", 1)
+		})
+
+		It("removes the users from the school", func() {
+			user1 := libregraph.NewEducationUser()
+			user1.SetId("user1")
+			user2 := libregraph.NewEducationUser()
+			user2.SetId("user2")
+			identityEducationBackend.On("GetEducationSchoolUsers", mock.Anything, mock.Anything, mock.Anything).Return([]*libregraph.EducationUser{user1, user2}, nil)
+			identityEducationBackend.On("DeleteEducationSchool", mock.Anything, mock.Anything, mock.Anything).Return(nil)
+			identityEducationBackend.On("RemoveUserFromEducationSchool", mock.Anything, mock.Anything, *user1.Id).Return(nil)
+			identityEducationBackend.On("RemoveUserFromEducationSchool", mock.Anything, mock.Anything, *user2.Id).Return(nil)
+
 			r := httptest.NewRequest(http.MethodPatch, "/graph/v1.0/education/schools", nil)
 			rctx := chi.NewRouteContext()
 			rctx.URLParams.Add("schoolID", *newSchool.Id)
@@ -344,6 +364,8 @@ var _ = Describe("Schools", func() {
 
 			Expect(rr.Code).To(Equal(http.StatusNoContent))
 			identityEducationBackend.AssertNumberOfCalls(GinkgoT(), "DeleteEducationSchool", 1)
+			identityEducationBackend.AssertNumberOfCalls(GinkgoT(), "RemoveUserFromEducationSchool", 2)
+			identityEducationBackend.AssertNumberOfCalls(GinkgoT(), "GetEducationSchoolUsers", 1)
 		})
 	})