Skip to content

Commit

Permalink
graph/education: Check school number for duplicates before adding a s…
Browse files Browse the repository at this point in the history 
…chool
  • Loading branch information
@rhafer
rhafer committed Sep 26, 2023
1 parent 594647f commit e00db0f
Show file tree
Hide file tree
Showing 3 changed files with 147 additions and 37 deletions.
6 changes: 6 additions & 0 deletions changelog/unreleased/fix-graph-education-createschool.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Bugfix: Check school number for duplicates before adding a school

We fixed an issue that allowed to create two schools with the same school number

https://github.com/owncloud/ocis/pull/7351
https://github.com/owncloud/enterprise/issues/6051
18 changes: 15 additions & 3 deletions services/graph/pkg/identity/ldap_education_school.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,19 @@ func (i *LDAP) CreateEducationSchool(ctx context.Context, school libregraph.Educ
return nil, ErrReadOnly
}

// Here we should verify that the school number is not already used
// Check that the school number is not already used
_, err := i.getSchoolByNumber(school.GetSchoolNumber())
switch err {
case nil:
msg := "A school with that number is already present"
logger.Warn().Str("schoolNumber", school.GetSchoolNumber()).Msg(msg)
return nil, errorcode.New(errorcode.NameAlreadyExists, msg)
case ErrNotFound:
break
default:
logger.Error().Err(err).Str("schoolNumber", school.GetSchoolNumber()).Msg("error looking up school by number")
return nil, errorcode.New(errorcode.GeneralException, "error looking up school by number")
}

attributeTypeAndValue := ldap.AttributeTypeAndValue{
Type: i.educationConfig.schoolAttributeMap.displayName,
Expand All @@ -141,14 +153,14 @@ func (i *LDAP) CreateEducationSchool(ctx context.Context, school libregraph.Educ
logger.Debug().Err(err).Msg("error adding school")
if errors.As(err, &lerr) {
if lerr.ResultCode == ldap.LDAPResultEntryAlreadyExists {
err = errorcode.New(errorcode.NameAlreadyExists, lerr.Error())
err = errorcode.New(errorcode.NameAlreadyExists, "A school with that name is already present")
}
}
return nil, err
}

// Read back school from LDAP to get the generated UUID
e, err := i.getSchoolByDN(ar.DN)
e, err = i.getSchoolByDN(ar.DN)
if err != nil {
return nil, err
}
Expand Down
160 changes: 126 additions & 34 deletions services/graph/pkg/identity/ldap_education_school_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ package identity

import (
"context"
"errors"
"testing"
"time"

"github.com/go-ldap/ldap/v3"
libregraph "github.com/owncloud/libre-graph-api-go"
"github.com/owncloud/ocis/v2/services/graph/mocks"
"github.com/owncloud/ocis/v2/services/graph/pkg/config"
"github.com/owncloud/ocis/v2/services/graph/pkg/service/v0/errorcode"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
)
Expand Down Expand Up @@ -67,45 +69,135 @@ var (
)

func TestCreateEducationSchool(t *testing.T) {
lm := &mocks.Client{}

ldapSchoolAddRequestMatcher := func(ar *ldap.AddRequest) bool {
if ar.DN != "ou=Test School," {
return false
}
for _, attr := range ar.Attributes {
if attr.Type == "ocEducationSchoolTerminationTimestamp" {
tests := []struct {
name string
schoolNumber string
schoolName string
expectedError error
}{
{
name: "Create a Education School succeeds",
schoolNumber: "0123",
schoolName: "Test School",
expectedError: nil,
}, {
name: "Create a Education School with a duplicated Schoolnumber fails with an error",
schoolNumber: "0666",
schoolName: "Test School",
expectedError: errorcode.New(errorcode.NameAlreadyExists, "A school with that number is already present"),
}, {
name: "Create a Education School with a duplicated Name fails with an error",
schoolNumber: "0123",
schoolName: "Existing Test School",
expectedError: errorcode.New(errorcode.NameAlreadyExists, "A school with that name is already present"),
}, {
name: "Create a Education School fails, when there is a backend error",
schoolNumber: "1111",
schoolName: "Test School",
expectedError: errorcode.New(errorcode.GeneralException, "error looking up school by number"),
},
}
for _, tt := range tests {
lm := &mocks.Client{}
ldapSchoolGoodAddRequestMatcher := func(ar *ldap.AddRequest) bool {
if ar.DN != "ou=Test School," {
return false
}
for _, attr := range ar.Attributes {
if attr.Type == "ocEducationSchoolTerminationTimestamp" {
return false
}
}
return true
}
return true
}

lm.On("Add", mock.MatchedBy(ldapSchoolAddRequestMatcher)).Return(nil)
lm.On("Add", mock.MatchedBy(ldapSchoolGoodAddRequestMatcher)).Return(nil)

lm.On("Search", mock.Anything).
Return(
&ldap.SearchResult{
Entries: []*ldap.Entry{schoolEntry},
},
nil)
ldapExistingSchoolAddRequestMatcher := func(ar *ldap.AddRequest) bool {
if ar.DN == "ou=Existing Test School," {
return true
}
return false
}
lm.On("Add", mock.MatchedBy(ldapExistingSchoolAddRequestMatcher)).Return(ldap.NewError(ldap.LDAPResultEntryAlreadyExists, errors.New("")))

b, err := getMockedBackend(lm, eduConfig, &logger)
assert.Nil(t, err)
assert.NotEqual(t, "", b.educationConfig.schoolObjectClass)
school := libregraph.NewEducationSchool()
school.SetDisplayName("Test School")
school.SetSchoolNumber("0123")
school.SetId("abcd-defg")
res_school, err := b.CreateEducationSchool(context.Background(), *school)
lm.AssertNumberOfCalls(t, "Add", 1)
lm.AssertNumberOfCalls(t, "Search", 1)
assert.Nil(t, err)
assert.NotNil(t, res_school)
assert.Equal(t, res_school.GetDisplayName(), school.GetDisplayName())
assert.Equal(t, res_school.GetId(), school.GetId())
assert.Equal(t, res_school.GetSchoolNumber(), school.GetSchoolNumber())
assert.False(t, res_school.HasTerminationDate())
schoolNumberSearchRequest := &ldap.SearchRequest{
BaseDN: "",
Scope: 2,
SizeLimit: 1,
Filter: "(&(objectClass=ocEducationSchool)(ocEducationSchoolNumber=0123))",
Attributes: []string{"ou", "owncloudUUID", "ocEducationSchoolNumber", "ocEducationSchoolTerminationTimestamp"},
Controls: []ldap.Control(nil),
}
lm.On("Search", schoolNumberSearchRequest).
Return(
&ldap.SearchResult{
Entries: []*ldap.Entry{},
},
nil)
existingSchoolNumberSearchRequest := &ldap.SearchRequest{
BaseDN: "",
Scope: 2,
SizeLimit: 1,
Filter: "(&(objectClass=ocEducationSchool)(ocEducationSchoolNumber=0666))",
Attributes: []string{"ou", "owncloudUUID", "ocEducationSchoolNumber", "ocEducationSchoolTerminationTimestamp"},
Controls: []ldap.Control(nil),
}
lm.On("Search", existingSchoolNumberSearchRequest).
Return(
&ldap.SearchResult{
Entries: []*ldap.Entry{schoolEntry},
},
nil)
schoolNumberSearchRequestError := &ldap.SearchRequest{
BaseDN: "",
Scope: 2,
SizeLimit: 1,
Filter: "(&(objectClass=ocEducationSchool)(ocEducationSchoolNumber=1111))",
Attributes: []string{"ou", "owncloudUUID", "ocEducationSchoolNumber", "ocEducationSchoolTerminationTimestamp"},
Controls: []ldap.Control(nil),
}
lm.On("Search", schoolNumberSearchRequestError).
Return(
&ldap.SearchResult{
Entries: []*ldap.Entry{},
},
ldap.NewError(ldap.LDAPResultOther, errors.New("some error")))
schoolLookupAfterCreate := &ldap.SearchRequest{
BaseDN: "ou=Test School,",
Scope: 0,
SizeLimit: 1,
Filter: "(objectClass=ocEducationSchool)",
Attributes: []string{"ou", "owncloudUUID", "ocEducationSchoolNumber", "ocEducationSchoolTerminationTimestamp"},
Controls: []ldap.Control(nil),
}
lm.On("Search", schoolLookupAfterCreate).
Return(
&ldap.SearchResult{
Entries: []*ldap.Entry{schoolEntry},
},
nil)
b, err := getMockedBackend(lm, eduConfig, &logger)
assert.Nil(t, err)
assert.NotEqual(t, "", b.educationConfig.schoolObjectClass)

school := libregraph.NewEducationSchool()
school.SetDisplayName(tt.schoolName)
school.SetSchoolNumber(tt.schoolNumber)
school.SetId("abcd-defg")
res_school, err := b.CreateEducationSchool(context.Background(), *school)
if tt.expectedError == nil {
assert.Nil(t, err)
lm.AssertNumberOfCalls(t, "Add", 1)
assert.NotNil(t, res_school)
assert.Equal(t, res_school.GetDisplayName(), school.GetDisplayName())
assert.Equal(t, res_school.GetId(), school.GetId())
assert.Equal(t, res_school.GetSchoolNumber(), school.GetSchoolNumber())
assert.False(t, res_school.HasTerminationDate())
} else {
assert.Equal(t, err, tt.expectedError)
assert.Nil(t, res_school)
}
}
}

func TestUpdateEducationSchoolTerminationDate(t *testing.T) {
Expand Down

0 comments on commit e00db0f

Please sign in to comment.