diff --git a/services/graph/pkg/identity/ldap.go b/services/graph/pkg/identity/ldap.go index dbe899bdbc..5224716be8 100644 --- a/services/graph/pkg/identity/ldap.go +++ b/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 { - 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 +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 +} diff --git a/services/graph/pkg/identity/ldap_education_school_test.go b/services/graph/pkg/identity/ldap_education_school_test.go index c1b4e3d2f5..bbd5439de4 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.go b/services/graph/pkg/identity/ldap_education_user.go index 8bafdd62f6..03e1d1824a 100644 --- a/services/graph/pkg/identity/ldap_education_user.go +++ b/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 } diff --git a/services/graph/pkg/identity/ldap_education_user_test.go b/services/graph/pkg/identity/ldap_education_user_test.go index 3e7d387174..fdd931d3df 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 8385ebf3f6..586c8abdb5 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)) }) }) })