From d133a8e4b2c3380f1702a8dff598e429b5d23031 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Thu, 9 Mar 2023 07:52:06 +0100 Subject: [PATCH] graph: Add missing patch support for education/users (#5772) * graph: Allow updating "surname" and "givenName" of users Also use attribute getters to lookup the attribute Values instead of fiddling around with pointers. * graph: Allow updating education users Update suppport for education users was still missing. --- services/graph/pkg/identity/ldap.go | 36 ++++-- .../identity/ldap_education_school_test.go | 6 +- .../graph/pkg/identity/ldap_education_user.go | 121 +++++++++++++++++- .../pkg/identity/ldap_education_user_test.go | 113 +++++++++++++++- 4 files changed, 253 insertions(+), 23 deletions(-) diff --git a/services/graph/pkg/identity/ldap.go b/services/graph/pkg/identity/ldap.go index 7d1d19e5f..5266e4f06 100644 --- a/services/graph/pkg/identity/ldap.go +++ b/services/graph/pkg/identity/ldap.go @@ -258,13 +258,13 @@ func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph. var updateNeeded bool // Don't allow updates of the ID - if user.Id != nil && *user.Id != "" { - if e.GetEqualFoldAttributeValue(i.userAttributeMap.id) != *user.Id { + if user.GetId() != "" { + if e.GetEqualFoldAttributeValue(i.userAttributeMap.id) != user.GetId() { return nil, errorcode.New(errorcode.NotAllowed, "changing the UserId is not allowed") } } - if user.OnPremisesSamAccountName != nil && *user.OnPremisesSamAccountName != "" { - if eu := e.GetEqualFoldAttributeValue(i.userAttributeMap.userName); eu != *user.OnPremisesSamAccountName { + if user.GetOnPremisesSamAccountName() != "" { + if eu := e.GetEqualFoldAttributeValue(i.userAttributeMap.userName); eu != user.GetOnPremisesSamAccountName() { e, err = i.changeUserName(ctx, e.DN, eu, user.GetOnPremisesSamAccountName()) if err != nil { return nil, err @@ -273,19 +273,31 @@ func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph. } mr := ldap.ModifyRequest{DN: e.DN} - if user.DisplayName != nil && *user.DisplayName != "" { - if e.GetEqualFoldAttributeValue(i.userAttributeMap.displayName) != *user.DisplayName { - mr.Replace(i.userAttributeMap.displayName, []string{*user.DisplayName}) + if user.GetDisplayName() != "" { + if e.GetEqualFoldAttributeValue(i.userAttributeMap.displayName) != user.GetDisplayName() { + mr.Replace(i.userAttributeMap.displayName, []string{user.GetDisplayName()}) updateNeeded = true } } - if user.Mail != nil && *user.Mail != "" { - if e.GetEqualFoldAttributeValue(i.userAttributeMap.mail) != *user.Mail { - mr.Replace(i.userAttributeMap.mail, []string{*user.Mail}) + if user.GetMail() != "" { + if e.GetEqualFoldAttributeValue(i.userAttributeMap.mail) != user.GetMail() { + mr.Replace(i.userAttributeMap.mail, []string{user.GetMail()}) updateNeeded = true } } - if user.PasswordProfile != nil && user.PasswordProfile.Password != nil && *user.PasswordProfile.Password != "" { + 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.PasswordProfile != nil && user.PasswordProfile.GetPassword() != "" { if i.usePwModifyExOp { if err := i.updateUserPassowrd(ctx, e.DN, user.PasswordProfile.GetPassword()); err != nil { return nil, err @@ -293,7 +305,7 @@ func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph. } else { // password are hashed server side there is no need to check if the new password // is actually different from the old one. - mr.Replace("userPassword", []string{*user.PasswordProfile.Password}) + mr.Replace("userPassword", []string{user.PasswordProfile.GetPassword()}) updateNeeded = true } } diff --git a/services/graph/pkg/identity/ldap_education_school_test.go b/services/graph/pkg/identity/ldap_education_school_test.go index a97d4804d..ffcbd918a 100644 --- a/services/graph/pkg/identity/ldap_education_school_test.go +++ b/services/graph/pkg/identity/ldap_education_school_test.go @@ -322,7 +322,7 @@ var userByIDSearch1 *ldap.SearchRequest = &ldap.SearchRequest{ Scope: 2, SizeLimit: 1, Filter: "(&(objectClass=ocEducationUser)(|(uid=abcd-defg)(entryUUID=abcd-defg)))", - Attributes: []string{"displayname", "entryUUID", "mail", "uid", "oCExternalIdentity", "userClass", "ocMemberOfSchool"}, + Attributes: eduUserAttrs, Controls: []ldap.Control(nil), } @@ -331,7 +331,7 @@ var userByIDSearch2 *ldap.SearchRequest = &ldap.SearchRequest{ Scope: 2, SizeLimit: 1, Filter: "(&(objectClass=ocEducationUser)(|(uid=does-not-exist)(entryUUID=does-not-exist)))", - Attributes: []string{"displayname", "entryUUID", "mail", "uid", "oCExternalIdentity", "userClass", "ocMemberOfSchool"}, + Attributes: eduUserAttrs, Controls: []ldap.Control(nil), } @@ -439,7 +439,7 @@ var usersBySchoolIDSearch *ldap.SearchRequest = &ldap.SearchRequest{ Scope: 2, SizeLimit: 0, Filter: "(&(objectClass=ocEducationUser)(ocMemberOfSchool=abcd-defg))", - Attributes: []string{"displayname", "entryUUID", "mail", "uid", "oCExternalIdentity", "userClass", "ocMemberOfSchool"}, + Attributes: eduUserAttrs, Controls: []ldap.Control(nil), } diff --git a/services/graph/pkg/identity/ldap_education_user.go b/services/graph/pkg/identity/ldap_education_user.go index e346bd989..a0b32e57d 100644 --- a/services/graph/pkg/identity/ldap_education_user.go +++ b/services/graph/pkg/identity/ldap_education_user.go @@ -77,7 +77,108 @@ func (i *LDAP) DeleteEducationUser(ctx context.Context, nameOrID string) error { // UpdateEducationUser applies changes to given education user, identified by username or id func (i *LDAP) UpdateEducationUser(ctx context.Context, nameOrID string, user libregraph.EducationUser) (*libregraph.EducationUser, error) { - return nil, errNotImplemented + logger := i.logger.SubloggerWithRequestID(ctx) + logger.Debug().Str("backend", "ldap").Msg("UpdateEducationUser") + if !i.writeEnabled { + return nil, ErrReadOnly + } + e, err := i.getEducationUserByNameOrID(nameOrID) + if err != nil { + return nil, err + } + + var updateNeeded bool + + // Don't allow updates of the ID + if user.GetId() != "" { + if e.GetEqualFoldAttributeValue(i.userAttributeMap.id) != user.GetId() { + return nil, errorcode.New(errorcode.NotAllowed, "changing the UserId is not allowed") + } + } + if user.GetOnPremisesSamAccountName() != "" { + if eu := e.GetEqualFoldAttributeValue(i.userAttributeMap.userName); eu != user.GetOnPremisesSamAccountName() { + e, err = i.changeUserName(ctx, e.DN, eu, user.GetOnPremisesSamAccountName()) + if err != nil { + return nil, err + } + e, err = i.getEducationUserByDN(e.DN) + if err != nil { + return nil, err + } + } + } + + 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.PasswordProfile != nil && user.PasswordProfile.GetPassword() != "" { + if i.usePwModifyExOp { + if err := i.updateUserPassowrd(ctx, e.DN, user.PasswordProfile.GetPassword()); err != nil { + return nil, err + } + } else { + // password are hashed server side there is no need to check if the new password + // is actually different from the old one. + mr.Replace("userPassword", []string{user.PasswordProfile.GetPassword()}) + 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 { + identityStr, err := i.identityToLDAPAttrValue(identity) + if err != nil { + return nil, err + } + attrValues = append(attrValues, identityStr) + } + mr.Replace(i.educationConfig.userAttributeMap.identities, attrValues) + updateNeeded = true + } + + if updateNeeded { + if err := i.conn.Modify(&mr); err != nil { + return nil, err + } + } + + // Read back user from LDAP to get the generated UUID + e, err = i.getEducationUserByDN(e.DN) + if err != nil { + return nil, err + } + + returnUser := i.createEducationUserModelFromLDAP(e) + + return returnUser, nil } // GetEducationUser implements the EducationBackend interface for the LDAP backend. @@ -189,11 +290,10 @@ func (i *LDAP) educationUserToLDAPAttrValues(user libregraph.EducationUser, attr } if identities, ok := user.GetIdentitiesOk(); ok { for _, identity := range identities { - // TODO add support for the "signInType" of objectIdentity - if identity.GetIssuer() == "" || identity.GetIssuerAssignedId() == "" { - return nil, fmt.Errorf("missing Attribute for objectIdentity") + identityStr, err := i.identityToLDAPAttrValue(identity) + if err != nil { + return nil, err } - identityStr := fmt.Sprintf(" $ %s $ %s", identity.GetIssuer(), identity.GetIssuerAssignedId()) attrs[i.educationConfig.userAttributeMap.identities] = append( attrs[i.educationConfig.userAttributeMap.identities], identityStr, @@ -203,6 +303,14 @@ func (i *LDAP) educationUserToLDAPAttrValues(user libregraph.EducationUser, attr attrs["objectClass"] = append(attrs["objectClass"], i.educationConfig.userObjectClass) return attrs, nil } +func (i *LDAP) identityToLDAPAttrValue(identity libregraph.ObjectIdentity) (string, error) { + // TODO add support for the "signInType" of objectIdentity + if identity.GetIssuer() == "" || identity.GetIssuerAssignedId() == "" { + return "", fmt.Errorf("missing Attribute for objectIdentity") + } + identityStr := fmt.Sprintf(" $ %s $ %s", identity.GetIssuer(), identity.GetIssuerAssignedId()) + return identityStr, nil +} func (i *LDAP) educationUserToAddRequest(user libregraph.EducationUser) (*ldap.AddRequest, error) { plainUser := i.educationUserToUser(user) @@ -234,6 +342,9 @@ func (i *LDAP) getEducationUserAttrTypes() []string { i.userAttributeMap.id, i.userAttributeMap.mail, i.userAttributeMap.userName, + i.userAttributeMap.surname, + i.userAttributeMap.givenName, + i.userAttributeMap.accountEnabled, i.educationConfig.userAttributeMap.identities, i.educationConfig.userAttributeMap.primaryRole, i.educationConfig.memberOfSchoolAttribute, diff --git a/services/graph/pkg/identity/ldap_education_user_test.go b/services/graph/pkg/identity/ldap_education_user_test.go index 960bec73b..dc4777e4d 100644 --- a/services/graph/pkg/identity/ldap_education_user_test.go +++ b/services/graph/pkg/identity/ldap_education_user_test.go @@ -11,6 +11,8 @@ import ( "github.com/test-go/testify/mock" ) +var eduUserAttrs = []string{"displayname", "entryUUID", "mail", "uid", "sn", "givenname", "userEnabledAttribute", "oCExternalIdentity", "userClass", "ocMemberOfSchool"} + var eduUserEntry = ldap.NewEntry("uid=user,ou=people,dc=test", map[string][]string{ "uid": {"testuser"}, @@ -23,6 +25,18 @@ var eduUserEntry = ldap.NewEntry("uid=user,ou=people,dc=test", "xxx $ http://idpnew $ xxxxx-xxxxx-xxxxx", }, }) +var renamedEduUserEntry = ldap.NewEntry("uid=newtestuser,ou=people,dc=test", + map[string][]string{ + "uid": {"newtestuser"}, + "displayname": {"Test User"}, + "mail": {"user@example"}, + "entryuuid": {"abcd-defg"}, + "userClass": {"student"}, + "oCExternalIdentity": { + "$ http://idp $ testuser", + "xxx $ http://idpnew $ xxxxx-xxxxx-xxxxx", + }, + }) var eduUserEntryWithSchool = ldap.NewEntry("uid=user,ou=people,dc=test", map[string][]string{ "uid": {"testuser"}, @@ -42,7 +56,7 @@ var sr1 *ldap.SearchRequest = &ldap.SearchRequest{ Scope: 2, SizeLimit: 1, Filter: "(&(objectClass=ocEducationUser)(|(uid=abcd-defg)(entryUUID=abcd-defg)))", - Attributes: []string{"displayname", "entryUUID", "mail", "uid", "oCExternalIdentity", "userClass", "ocMemberOfSchool"}, + Attributes: eduUserAttrs, Controls: []ldap.Control(nil), } var sr2 *ldap.SearchRequest = &ldap.SearchRequest{ @@ -50,7 +64,7 @@ var sr2 *ldap.SearchRequest = &ldap.SearchRequest{ Scope: 2, SizeLimit: 1, Filter: "(&(objectClass=ocEducationUser)(|(uid=xxxx-xxxx)(entryUUID=xxxx-xxxx)))", - Attributes: []string{"displayname", "entryUUID", "mail", "uid", "oCExternalIdentity", "userClass", "ocMemberOfSchool"}, + Attributes: eduUserAttrs, Controls: []ldap.Control(nil), } @@ -133,7 +147,7 @@ func TestGetEducationUsers(t *testing.T) { Scope: 2, SizeLimit: 0, Filter: "(objectClass=ocEducationUser)", - Attributes: []string{"displayname", "entryUUID", "mail", "uid", "oCExternalIdentity", "userClass", "ocMemberOfSchool"}, + Attributes: eduUserAttrs, Controls: []ldap.Control(nil), } lm.On("Search", sr).Return(&ldap.SearchResult{Entries: []*ldap.Entry{eduUserEntry}}, nil) @@ -143,3 +157,96 @@ func TestGetEducationUsers(t *testing.T) { lm.AssertNumberOfCalls(t, "Search", 1) assert.Nil(t, err) } + +func TestUpdateEducationUser(t *testing.T) { + lm := &mocks.Client{} + b, err := getMockedBackend(lm, eduConfig, &logger) + assert.Nil(t, err) + userSearchReq := &ldap.SearchRequest{ + BaseDN: "ou=people,dc=test", + Scope: 2, + SizeLimit: 1, + Filter: "(&(objectClass=ocEducationUser)(|(uid=testuser)(entryUUID=testuser)))", + Attributes: eduUserAttrs, + } + userLookupReq := &ldap.SearchRequest{ + BaseDN: "uid=newtestuser,ou=people,dc=test", + Scope: 0, + SizeLimit: 1, + Filter: "(objectClass=inetOrgPerson)", + Attributes: []string{"displayname", "entryUUID", "mail", "uid", "sn", "givenname", "userEnabledAttribute"}, + } + eduUserLookupReq := &ldap.SearchRequest{ + BaseDN: "uid=newtestuser,ou=people,dc=test", + Scope: 0, + SizeLimit: 1, + Filter: "(objectClass=ocEducationUser)", + Attributes: eduUserAttrs, + } + groupSearchReq := &ldap.SearchRequest{ + BaseDN: "ou=groups,dc=test", + Scope: 2, + Filter: "(&(objectClass=groupOfNames)(member=uid=user,ou=people,dc=test))", + Attributes: []string{ + "cn", + "entryUUID", + }, + } + lm.On("Search", userLookupReq). + Return( + &ldap.SearchResult{ + Entries: []*ldap.Entry{ + renamedEduUserEntry, + }, + }, + nil) + lm.On("Search", eduUserLookupReq). + Return( + &ldap.SearchResult{ + Entries: []*ldap.Entry{ + renamedEduUserEntry, + }, + }, + nil) + lm.On("Search", userSearchReq). + Return( + &ldap.SearchResult{ + Entries: []*ldap.Entry{ + eduUserEntry, + }, + }, + nil) + lm.On("Search", groupSearchReq). + Return( + &ldap.SearchResult{ + Entries: []*ldap.Entry{}, + }, + nil) + modReq := ldap.ModifyRequest{ + DN: "uid=newtestuser,ou=people,dc=test", + Changes: []ldap.Change{ + { + Operation: ldap.ReplaceAttribute, + Modification: ldap.PartialAttribute{ + Type: "mail", + Vals: []string{"new@mail.org"}, + }, + }, + }, + } + modDNReq := ldap.ModifyDNRequest{ + DN: "uid=user,ou=people,dc=test", + NewRDN: "uid=newtestuser", + DeleteOldRDN: true, + } + lm.On("ModifyDN", &modDNReq).Return(nil) + lm.On("Modify", &modReq).Return(nil) + user := libregraph.NewEducationUser() + user.SetOnPremisesSamAccountName("newtestuser") + user.SetMail("new@mail.org") + 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()) +}