Repository: owncloud/ocis (go)

Issue:
graph: Support `accountEnabled` property on education/users/
## Describe the bug

The `accountEnabled` property, while expose on the API is currently no correctly update when PATCHing it via the /education/users endpoint.

Pull request:
graph: Fix so that accountEnabled updates work for educationUser.
## Description
Fix so that the accountEnabled attribute will be updated on PATCH requests for educationUser.

## Related Issue
- Fixes #5816 

## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->

## 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. -->
Tested using curl and updated unit tests.

## 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
- [ ] Unit tests added
- [ ] Acceptance tests added
- [ ] Documentation ticket raised: <link> 


Patch:
diff --git a/services/graph/pkg/identity/ldap.go b/services/graph/pkg/identity/ldap.go
index dbe899bdbcb..696d1647772 100644
--- a/services/graph/pkg/identity/ldap.go
+++ b/services/graph/pkg/identity/ldap.go
@@ -274,36 +274,23 @@ func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph.
 	}
 
 	mr := ldap.ModifyRequest{DN: e.DN}
-	if user.GetDisplayName() != "" {
-		if e.GetEqualFoldAttributeValue(i.userAttributeMap.displayName) != user.GetDisplayName() {
-			mr.Replace(i.userAttributeMap.displayName, []string{user.GetDisplayName()})
-			updateNeeded = true
-		}
-	}
-	if user.GetMail() != "" {
-		if e.GetEqualFoldAttributeValue(i.userAttributeMap.mail) != user.GetMail() {
-			mr.Replace(i.userAttributeMap.mail, []string{user.GetMail()})
-			updateNeeded = true
-		}
-	}
-	if user.GetSurname() != "" {
-		if e.GetEqualFoldAttributeValue(i.userAttributeMap.surname) != user.GetSurname() {
-			mr.Replace(i.userAttributeMap.surname, []string{user.GetSurname()})
-			updateNeeded = true
-		}
-	}
-	if user.GetGivenName() != "" {
-		if e.GetEqualFoldAttributeValue(i.userAttributeMap.givenName) != user.GetGivenName() {
-			mr.Replace(i.userAttributeMap.givenName, []string{user.GetGivenName()})
-			updateNeeded = true
-		}
-	}
-	if user.GetUserType() != "" {
-		if e.GetEqualFoldAttributeValue(i.userAttributeMap.userType) != user.GetUserType() {
-			mr.Replace(i.userAttributeMap.userType, []string{user.GetUserType()})
-			updateNeeded = true
+	properties := map[string]string{
+		i.userAttributeMap.displayName: user.GetDisplayName(),
+		i.userAttributeMap.mail:        user.GetMail(),
+		i.userAttributeMap.surname:     user.GetSurname(),
+		i.userAttributeMap.givenName:   user.GetGivenName(),
+		i.userAttributeMap.userType:    user.GetUserType(),
+	}
+
+	for attribute, value := range properties {
+		if value != "" {
+			if e.GetEqualFoldAttributeValue(attribute) != value {
+				mr.Replace(attribute, []string{value})
+				updateNeeded = true
+			}
 		}
 	}
+
 	if user.PasswordProfile != nil && user.PasswordProfile.GetPassword() != "" {
 		if i.usePwModifyExOp {
 			if err := i.updateUserPassowrd(ctx, e.DN, user.PasswordProfile.GetPassword()); err != nil {
@@ -322,28 +309,13 @@ func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph.
 	// "attribute": For the upstream user management service which modifies accountEnabled on the user entry
 	// "group": Makes it possible for local admins to disable users by adding them to a special group
 	if user.AccountEnabled != nil {
-		switch i.disableUserMechanism {
-		case DisableMechanismNone:
-			return nil, errors.New("accountEnabled option not compatible with backend disable user mechanism")
-		case DisableMechanismAttribute:
-			boolString := strings.ToUpper(strconv.FormatBool(user.GetAccountEnabled()))
-			ldapValue := e.GetEqualFoldAttributeValue(i.userAttributeMap.accountEnabled)
-			if ldapValue != "" {
-				mr.Replace(i.userAttributeMap.accountEnabled, []string{boolString})
-			} else {
-				mr.Add(i.userAttributeMap.accountEnabled, []string{boolString})
-			}
-			updateNeeded = true
-		case DisableMechanismGroup:
-			if user.GetAccountEnabled() {
-				err = i.enableUser(logger, e.DN)
-			} else {
-				err = i.disableUser(logger, e.DN)
-			}
+		un, err := i.updateAccountEnabledState(logger, user.GetAccountEnabled(), e, &mr)
 
-			if err != nil {
-				return nil, err
-			}
+		if err != nil {
+			return nil, err
+		}
+
+		if un {
 			updateNeeded = true
 		}
 	}
@@ -364,7 +336,7 @@ func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph.
 
 	// To avoid an ldap lookup for group membership, set the enabled flag to same as input value
 	// since this would have been updated with group membership from the input anyway.
-	if user.AccountEnabled != nil && i.disableUserMechanism != DisableMechanismNone {
+	if user.AccountEnabled != nil && i.disableUserMechanism == DisableMechanismGroup {
 		returnUser.AccountEnabled = user.AccountEnabled
 	}
 
@@ -1057,3 +1029,32 @@ func (i *LDAP) usersEnabledState(users []*ldap.Entry) (usersEnabledState map[str
 
 	return usersEnabledState, nil
 }
+
+// Behavior of enabling/disabling of users depends on the "disableUserMechanism" config option:
+//
+// "attribute": For the upstream user management service which modifies accountEnabled on the user entry
+// "group": Makes it possible for local admins to disable users by adding them to a special group
+func (i *LDAP) updateAccountEnabledState(logger log.Logger, accountEnabled bool, e *ldap.Entry, mr *ldap.ModifyRequest) (updateNeeded bool, err error) {
+	switch i.disableUserMechanism {
+	case DisableMechanismNone:
+		err = errors.New("accountEnabled option not compatible with backend disable user mechanism")
+	case DisableMechanismAttribute:
+		boolString := strings.ToUpper(strconv.FormatBool(accountEnabled))
+		ldapValue := e.GetEqualFoldAttributeValue(i.userAttributeMap.accountEnabled)
+		if ldapValue != "" {
+			mr.Replace(i.userAttributeMap.accountEnabled, []string{boolString})
+		} else {
+			mr.Add(i.userAttributeMap.accountEnabled, []string{boolString})
+		}
+		updateNeeded = true
+	case DisableMechanismGroup:
+		if accountEnabled {
+			err = i.enableUser(logger, e.DN)
+		} else {
+			err = i.disableUser(logger, e.DN)
+		}
+		updateNeeded = true
+	}
+
+	return updateNeeded, err
+}
diff --git a/services/graph/pkg/identity/ldap_education_user.go b/services/graph/pkg/identity/ldap_education_user.go
index 8bafdd62f63..9c9a8cbae7a 100644
--- a/services/graph/pkg/identity/ldap_education_user.go
+++ b/services/graph/pkg/identity/ldap_education_user.go
@@ -109,33 +109,32 @@ func (i *LDAP) UpdateEducationUser(ctx context.Context, nameOrID string, user li
 	}
 
 	mr := ldap.ModifyRequest{DN: e.DN}
-	if user.GetDisplayName() != "" {
-		if e.GetEqualFoldAttributeValue(i.userAttributeMap.displayName) != user.GetDisplayName() {
-			mr.Replace(i.userAttributeMap.displayName, []string{user.GetDisplayName()})
-			updateNeeded = true
-		}
-	}
-	if user.GetMail() != "" {
-		if e.GetEqualFoldAttributeValue(i.userAttributeMap.mail) != user.GetMail() {
-			mr.Replace(i.userAttributeMap.mail, []string{user.GetMail()})
-			updateNeeded = true
-		}
-	}
-	if user.GetSurname() != "" {
-		if e.GetEqualFoldAttributeValue(i.userAttributeMap.surname) != user.GetSurname() {
-			mr.Replace(i.userAttributeMap.surname, []string{user.GetSurname()})
-			updateNeeded = true
+	properties := map[string]string{
+		i.userAttributeMap.displayName:                 user.GetDisplayName(),
+		i.userAttributeMap.mail:                        user.GetMail(),
+		i.userAttributeMap.surname:                     user.GetSurname(),
+		i.userAttributeMap.givenName:                   user.GetGivenName(),
+		i.userAttributeMap.userType:                    user.GetUserType(),
+		i.educationConfig.userAttributeMap.primaryRole: user.GetPrimaryRole(),
+	}
+
+	for attribute, value := range properties {
+		if value != "" {
+			if e.GetEqualFoldAttributeValue(attribute) != value {
+				mr.Replace(attribute, []string{value})
+				updateNeeded = true
+			}
 		}
 	}
-	if user.GetGivenName() != "" {
-		if e.GetEqualFoldAttributeValue(i.userAttributeMap.givenName) != user.GetGivenName() {
-			mr.Replace(i.userAttributeMap.givenName, []string{user.GetGivenName()})
-			updateNeeded = true
+
+	if user.AccountEnabled != nil {
+		un, err := i.updateAccountEnabledState(logger, user.GetAccountEnabled(), e, &mr)
+
+		if err != nil {
+			return nil, err
 		}
-	}
-	if user.GetUserType() != "" {
-		if e.GetEqualFoldAttributeValue(i.userAttributeMap.userType) != user.GetUserType() {
-			mr.Replace(i.userAttributeMap.userType, []string{user.GetUserType()})
+
+		if un {
 			updateNeeded = true
 		}
 	}
@@ -151,12 +150,6 @@ func (i *LDAP) UpdateEducationUser(ctx context.Context, nameOrID string, user li
 			updateNeeded = true
 		}
 	}
-	if user.GetPrimaryRole() != "" {
-		if e.GetEqualFoldAttributeValue(i.educationConfig.userAttributeMap.primaryRole) != user.GetPrimaryRole() {
-			mr.Replace(i.educationConfig.userAttributeMap.primaryRole, []string{user.GetPrimaryRole()})
-			updateNeeded = true
-		}
-	}
 	if identities, ok := user.GetIdentitiesOk(); ok {
 		attrValues := make([]string, 0, len(identities))
 		for _, identity := range identities {
@@ -184,6 +177,12 @@ func (i *LDAP) UpdateEducationUser(ctx context.Context, nameOrID string, user li
 
 	returnUser := i.createEducationUserModelFromLDAP(e)
 
+	// To avoid an ldap lookup for group membership, set the enabled flag to same as input value
+	// since this would have been updated with group membership from the input anyway.
+	if user.AccountEnabled != nil && i.disableUserMechanism == DisableMechanismGroup {
+		returnUser.AccountEnabled = user.AccountEnabled
+	}
+
 	return returnUser, nil
 }
 


Test patch:
diff --git a/services/graph/pkg/identity/ldap_education_school_test.go b/services/graph/pkg/identity/ldap_education_school_test.go
index c1b4e3d2f52..bbd5439de45 100644
--- a/services/graph/pkg/identity/ldap_education_school_test.go
+++ b/services/graph/pkg/identity/ldap_education_school_test.go
@@ -22,6 +22,7 @@ var eduConfig = config.LDAP{
 	UserEmailAttribute:       "mail",
 	UserNameAttribute:        "uid",
 	UserEnabledAttribute:     "userEnabledAttribute",
+	DisableUserMechanism:     "attribute",
 	UserTypeAttribute:        "userTypeAttribute",
 
 	GroupBaseDN:        "ou=groups,dc=test",
diff --git a/services/graph/pkg/identity/ldap_education_user_test.go b/services/graph/pkg/identity/ldap_education_user_test.go
index 3e7d3871747..fdd931d3dfe 100644
--- a/services/graph/pkg/identity/ldap_education_user_test.go
+++ b/services/graph/pkg/identity/ldap_education_user_test.go
@@ -36,7 +36,8 @@ var eduUserEntry = ldap.NewEntry("uid=user,ou=people,dc=test",
 			"$ http://idp $ testuser",
 			"xxx $ http://idpnew $ xxxxx-xxxxx-xxxxx",
 		},
-		"userTypeAttribute": {"Member"},
+		"userTypeAttribute":    {"Member"},
+		"userEnabledAttribute": {"FALSE"},
 	})
 var renamedEduUserEntry = ldap.NewEntry("uid=newtestuser,ou=people,dc=test",
 	map[string][]string{
@@ -49,7 +50,8 @@ var renamedEduUserEntry = ldap.NewEntry("uid=newtestuser,ou=people,dc=test",
 			"$ http://idp $ testuser",
 			"xxx $ http://idpnew $ xxxxx-xxxxx-xxxxx",
 		},
-		"userTypeAttribute": {"Guest"},
+		"userTypeAttribute":    {"Guest"},
+		"userEnabledAttribute": {"TRUE"},
 	})
 var eduUserEntryWithSchool = ldap.NewEntry("uid=user,ou=people,dc=test",
 	map[string][]string{
@@ -103,6 +105,7 @@ func TestCreateEducationUser(t *testing.T) {
 	user.SetMail("testuser@example.org")
 	user.SetPrimaryRole("student")
 	user.SetUserType(("Member"))
+	user.SetAccountEnabled(false)
 	eduUser, err := b.CreateEducationUser(context.Background(), *user)
 	lm.AssertNumberOfCalls(t, "Add", 1)
 	lm.AssertNumberOfCalls(t, "Search", 1)
@@ -113,6 +116,7 @@ func TestCreateEducationUser(t *testing.T) {
 	assert.Equal(t, "abcd-defg", eduUser.GetId())
 	assert.Equal(t, eduUser.GetPrimaryRole(), user.GetPrimaryRole())
 	assert.Equal(t, eduUser.GetUserType(), user.GetUserType())
+	assert.Equal(t, eduUser.GetAccountEnabled(), false)
 }
 
 func TestDeleteEducationUser(t *testing.T) {
@@ -248,6 +252,13 @@ func TestUpdateEducationUser(t *testing.T) {
 					Vals: []string{"new@mail.org"},
 				},
 			},
+			{
+				Operation: ldap.ReplaceAttribute,
+				Modification: ldap.PartialAttribute{
+					Type: "userEnabledAttribute",
+					Vals: []string{"TRUE"},
+				},
+			},
 		},
 	}
 	modDNReq := ldap.ModifyDNRequest{
@@ -260,10 +271,12 @@ func TestUpdateEducationUser(t *testing.T) {
 	user := libregraph.NewEducationUser()
 	user.SetOnPremisesSamAccountName("newtestuser")
 	user.SetMail("new@mail.org")
+	user.SetAccountEnabled(true)
 	eduUser, err := b.UpdateEducationUser(context.Background(), "testuser", *user)
 	assert.NotNil(t, eduUser)
 	assert.Nil(t, err)
 	assert.Equal(t, eduUser.GetOnPremisesSamAccountName(), "newtestuser")
 	assert.Equal(t, "abcd-defg", eduUser.GetId())
 	assert.Equal(t, "Guest", eduUser.GetUserType())
+	assert.Equal(t, eduUser.GetAccountEnabled(), true)
 }
diff --git a/services/graph/pkg/service/v0/educationuser_test.go b/services/graph/pkg/service/v0/educationuser_test.go
index 8385ebf3f6e..586c8abdb50 100644
--- a/services/graph/pkg/service/v0/educationuser_test.go
+++ b/services/graph/pkg/service/v0/educationuser_test.go
@@ -449,6 +449,7 @@ var _ = Describe("EducationUsers", func() {
 			user.SetOnPremisesSamAccountName("user")
 			user.SetMail("user@example.com")
 			user.SetId("/users/user")
+			user.SetAccountEnabled(true)
 
 			identityEducationBackend.On("GetEducationUser", mock.Anything, mock.Anything, mock.Anything).Return(&user, nil)
 		})
@@ -502,6 +503,7 @@ var _ = Describe("EducationUsers", func() {
 			identityEducationBackend.On("UpdateEducationUser", mock.Anything, user.GetId(), mock.Anything).Return(user, nil)
 
 			user.SetDisplayName("New Display Name")
+			user.SetAccountEnabled(false)
 			data, err := json.Marshal(user)
 			Expect(err).ToNot(HaveOccurred())
 
@@ -519,6 +521,7 @@ var _ = Describe("EducationUsers", func() {
 			err = json.Unmarshal(data, &updatedUser)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(updatedUser.GetDisplayName()).To(Equal("New Display Name"))
+			Expect(updatedUser.GetAccountEnabled()).To(Equal(false))
 		})
 	})
 })