graph: Fix so that accountEnabled updates work for educationUser.

services/graph/pkg/identity/ldap.go

@@ -322,28 +322,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 {
+		un, err := i.updateAccountEnabledState(logger, user.GetAccountEnabled(), e, &mr)
-		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)
-			}
 
-			if err != nil {
+		if err != nil {
-				return nil, err
+			return nil, err
-			}
+		}
+
+		if un {
 			updateNeeded = true
 		}
 	}
@@ -364,7 +349,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 +1042,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
+}

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",

services/graph/pkg/identity/ldap_education_user.go

@@ -139,6 +139,17 @@ func (i *LDAP) UpdateEducationUser(ctx context.Context, nameOrID string, user li
 			updateNeeded = true
 		}
 	}
+	if user.AccountEnabled != nil {
+		un, err := i.updateAccountEnabledState(logger, user.GetAccountEnabled(), e, &mr)
+
+		if err != nil {
+			return nil, err
+		}
+
+		if un {
+			updateNeeded = true
+		}
+	}
 	if user.PasswordProfile != nil && user.PasswordProfile.GetPassword() != "" {
 		if i.usePwModifyExOp {
 			if err := i.updateUserPassowrd(ctx, e.DN, user.PasswordProfile.GetPassword()); err != nil {
@@ -184,6 +195,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
 }
 

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)
 }

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))
 		})
 	})
 })