From 525638588d64c427c44845984b7efde0d59fa39a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sw=C3=A4rd?= Date: Wed, 15 Mar 2023 11:41:17 +0100 Subject: [PATCH 1/2] graph: Fix so that accountEnabled updates work for educationUser. --- services/graph/pkg/identity/ldap.go | 58 ++++++++++++------- .../identity/ldap_education_school_test.go | 1 + .../graph/pkg/identity/ldap_education_user.go | 17 ++++++ .../pkg/identity/ldap_education_user_test.go | 17 +++++- .../pkg/service/v0/educationuser_test.go | 3 + 5 files changed, 72 insertions(+), 24 deletions(-) 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)) }) }) }) From c765e904df3c60d3b641cca74679a33dfa7ae3a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sw=C3=A4rd?= Date: Fri, 17 Mar 2023 11:03:38 +0100 Subject: [PATCH 2/2] Refactor user update methods to be a bit nicer. --- services/graph/pkg/identity/ldap.go | 43 ++++++---------- .../graph/pkg/identity/ldap_education_user.go | 50 ++++++------------- 2 files changed, 31 insertions(+), 62 deletions(-) diff --git a/services/graph/pkg/identity/ldap.go b/services/graph/pkg/identity/ldap.go index 5224716be8..696d164777 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 { diff --git a/services/graph/pkg/identity/ldap_education_user.go b/services/graph/pkg/identity/ldap_education_user.go index 03e1d1824a..9c9a8cbae7 100644 --- a/services/graph/pkg/identity/ldap_education_user.go +++ b/services/graph/pkg/identity/ldap_education_user.go @@ -109,36 +109,24 @@ 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 - } - } - 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(), + 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.AccountEnabled != nil { un, err := i.updateAccountEnabledState(logger, user.GetAccountEnabled(), e, &mr) @@ -162,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 {