From 6b11f0bfe47f76f62beadc76ad5015b9c69532e7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Sw=C3=A4rd?= <excds@kth.se>
Date: Tue, 14 Mar 2023 13:23:31 +0100
Subject: [PATCH] graph: Fix problem with unescaped semicolon and such causing
 problems.

---
 ocis-pkg/ldap/ldap.go                         | 32 -------------------
 ocis-pkg/ldap/ldap_test.go                    | 25 ---------------
 services/graph/pkg/identity/ldap.go           | 21 ++++++++----
 .../pkg/identity/ldap_education_class.go      |  7 ++--
 .../pkg/identity/ldap_education_school.go     | 22 +++++++------
 services/graph/pkg/identity/ldap_group.go     |  7 ++--
 6 files changed, 36 insertions(+), 78 deletions(-)
 delete mode 100644 ocis-pkg/ldap/ldap_test.go

diff --git a/ocis-pkg/ldap/ldap.go b/ocis-pkg/ldap/ldap.go
index 194ddb42ed..f9b5786b79 100644
--- a/ocis-pkg/ldap/ldap.go
+++ b/ocis-pkg/ldap/ldap.go
@@ -4,7 +4,6 @@ import (
 	"crypto/x509"
 	"errors"
 	"os"
-	"strings"
 	"time"
 
 	"github.com/owncloud/ocis/v2/ocis-pkg/log"
@@ -15,20 +14,6 @@ const (
 	caCheckSleep   = 2
 )
 
-var (
-	dnEscaper = strings.NewReplacer(
-		"\\", "\\\\",
-		",", "\\,",
-		"+", "\\+",
-		`"`, `\\"`,
-		"<", "\\<",
-		">", "\\>",
-		";", "\\;",
-		"=", "\\=",
-		"\000", "\\00",
-	)
-)
-
 func WaitForCA(log log.Logger, insecure bool, caCert string) error {
 	if !insecure && caCert != "" {
 		for i := 0; i < caCheckRetries; i++ {
@@ -52,20 +37,3 @@ func WaitForCA(log log.Logger, insecure bool, caCert string) error {
 	}
 	return nil
 }
-
-// EscapeDNAttributeValue escapes special characters in an attribute value as [described in RFC4514](https://datatracker.ietf.org/doc/html/rfc4514).
-func EscapeDNAttributeValue(v string) string {
-	if v == "" {
-		return v
-	}
-
-	v = dnEscaper.Replace(v)
-
-	if strings.HasSuffix(v, " ") {
-		v = v[:len(v)-1] + "\\ "
-	}
-	if strings.HasPrefix(v, "#") || strings.HasPrefix(v, " ") {
-		v = "\\" + v
-	}
-	return v
-}
diff --git a/ocis-pkg/ldap/ldap_test.go b/ocis-pkg/ldap/ldap_test.go
deleted file mode 100644
index d3ee3152cf..0000000000
--- a/ocis-pkg/ldap/ldap_test.go
+++ /dev/null
@@ -1,25 +0,0 @@
-package ldap_test
-
-import (
-	. "github.com/onsi/ginkgo/v2"
-	. "github.com/onsi/gomega"
-	"github.com/owncloud/ocis/v2/ocis-pkg/ldap"
-)
-
-var _ = Describe("ldap", func() {
-	DescribeTable("EscapeDNAttributeValue should escape special characters",
-		func(input, expected string) {
-			escaped := ldap.EscapeDNAttributeValue(input)
-			Expect(escaped).To(Equal(expected))
-		},
-		Entry("normal dn", "foobar", "foobar"),
-		Entry("including comma", "foo,bar", "foo\\,bar"),
-		Entry("including equals", "foo=bar", "foo\\=bar"),
-		Entry("beginning with number sign", "#foobar", "\\#foobar"),
-		Entry("beginning with space", " foobar", "\\ foobar"),
-		Entry("only one space", " ", "\\ "),
-		Entry("two spaces", "  ", "\\ \\ "),
-		Entry("ending with space", "foobar ", "foobar\\ "),
-		Entry("containing multiple special chars", "f+o>o,b<a;r=\"\000", `f\+o\>o\,b\<a\;r\=\\"\00`),
-	)
-})
diff --git a/services/graph/pkg/identity/ldap.go b/services/graph/pkg/identity/ldap.go
index 5266e4f06e..4a5dffbb47 100644
--- a/services/graph/pkg/identity/ldap.go
+++ b/services/graph/pkg/identity/ldap.go
@@ -12,7 +12,6 @@ import (
 	"github.com/gofrs/uuid"
 	"github.com/libregraph/idm/pkg/ldapdn"
 	libregraph "github.com/owncloud/libre-graph-api-go"
-	oldap "github.com/owncloud/ocis/v2/ocis-pkg/ldap"
 	"github.com/owncloud/ocis/v2/ocis-pkg/log"
 	"github.com/owncloud/ocis/v2/services/graph/pkg/config"
 	"github.com/owncloud/ocis/v2/services/graph/pkg/service/v0/errorcode"
@@ -596,14 +595,18 @@ func (i *LDAP) GetUsers(ctx context.Context, oreq *godata.GoDataRequest) ([]*lib
 func (i *LDAP) changeUserName(ctx context.Context, dn, originalUserName, newUserName string) (*ldap.Entry, error) {
 	logger := i.logger.SubloggerWithRequestID(ctx)
 
-	newDN := fmt.Sprintf("%s=%s", i.userAttributeMap.userName, newUserName)
+	attributeTypeAndValue := ldap.AttributeTypeAndValue{
+		Type:  i.userAttributeMap.userName,
+		Value: newUserName,
+	}
+	newDNString := attributeTypeAndValue.String()
 
-	logger.Debug().Str("originalDN", dn).Str("newDN", newDN).Msg("Modifying DN")
-	mrdn := ldap.NewModifyDNRequest(dn, newDN, true, "")
+	logger.Debug().Str("originalDN", dn).Str("newDN", newDNString).Msg("Modifying DN")
+	mrdn := ldap.NewModifyDNRequest(dn, newDNString, true, "")
 
 	if err := i.conn.ModifyDN(mrdn); err != nil {
 		var lerr *ldap.Error
-		logger.Debug().Str("originalDN", dn).Str("newDN", newDN).Err(err).Msg("Failed to modify DN")
+		logger.Debug().Str("originalDN", dn).Str("newDN", newDNString).Err(err).Msg("Failed to modify DN")
 		if errors.As(err, &lerr) {
 			if lerr.ResultCode == ldap.LDAPResultEntryAlreadyExists {
 				err = errorcode.New(errorcode.NameAlreadyExists, lerr.Error())
@@ -617,7 +620,7 @@ func (i *LDAP) changeUserName(ctx context.Context, dn, originalUserName, newUser
 		return nil, err
 	}
 
-	newFullDN, err := replaceDN(parsed, newDN)
+	newFullDN, err := replaceDN(parsed, newDNString)
 	if err != nil {
 		return nil, err
 	}
@@ -779,7 +782,11 @@ func (i *LDAP) getUserAttrTypes() []string {
 }
 
 func (i *LDAP) getUserLDAPDN(user libregraph.User) string {
-	return fmt.Sprintf("uid=%s,%s", oldap.EscapeDNAttributeValue(*user.OnPremisesSamAccountName), i.userBaseDN)
+	attributeTypeAndValue := ldap.AttributeTypeAndValue{
+		Type:  "uid",
+		Value: *user.OnPremisesSamAccountName,
+	}
+	return fmt.Sprintf("%s,%s", attributeTypeAndValue.String(), i.userBaseDN)
 }
 
 func (i *LDAP) userToAddRequest(user libregraph.User) (*ldap.AddRequest, error) {
diff --git a/services/graph/pkg/identity/ldap_education_class.go b/services/graph/pkg/identity/ldap_education_class.go
index 52856f7741..ee65eb970d 100644
--- a/services/graph/pkg/identity/ldap_education_class.go
+++ b/services/graph/pkg/identity/ldap_education_class.go
@@ -8,7 +8,6 @@ import (
 	"github.com/go-ldap/ldap/v3"
 	"github.com/libregraph/idm/pkg/ldapdn"
 	libregraph "github.com/owncloud/libre-graph-api-go"
-	oldap "github.com/owncloud/ocis/v2/ocis-pkg/ldap"
 	"github.com/owncloud/ocis/v2/services/graph/pkg/service/v0/errorcode"
 )
 
@@ -334,7 +333,11 @@ func (i *LDAP) groupToEducationClass(group libregraph.Group, e *ldap.Entry) *lib
 }
 
 func (i *LDAP) getEducationClassLDAPDN(class libregraph.EducationClass) string {
-	return fmt.Sprintf("ocEducationExternalId=%s,%s", oldap.EscapeDNAttributeValue(class.GetExternalId()), i.groupBaseDN)
+	attributeTypeAndValue := ldap.AttributeTypeAndValue{
+		Type:  "ocEducationExternalId",
+		Value: class.GetExternalId(),
+	}
+	return fmt.Sprintf("%s,%s", attributeTypeAndValue.String(), i.groupBaseDN)
 }
 
 func (i *LDAP) getEducationClassByID(nameOrID string, requestMembers bool) (*ldap.Entry, error) {
diff --git a/services/graph/pkg/identity/ldap_education_school.go b/services/graph/pkg/identity/ldap_education_school.go
index 884366d386..48858ab344 100644
--- a/services/graph/pkg/identity/ldap_education_school.go
+++ b/services/graph/pkg/identity/ldap_education_school.go
@@ -8,7 +8,6 @@ import (
 	"github.com/go-ldap/ldap/v3"
 	"github.com/gofrs/uuid"
 	libregraph "github.com/owncloud/libre-graph-api-go"
-	oldap "github.com/owncloud/ocis/v2/ocis-pkg/ldap"
 	"github.com/owncloud/ocis/v2/ocis-pkg/log"
 	"github.com/owncloud/ocis/v2/services/graph/pkg/config"
 	"github.com/owncloud/ocis/v2/services/graph/pkg/service/v0/errorcode"
@@ -112,9 +111,13 @@ func (i *LDAP) CreateEducationSchool(ctx context.Context, school libregraph.Educ
 
 	// Here we should verify that the school number is not already used
 
-	dn := fmt.Sprintf("%s=%s,%s",
-		i.educationConfig.schoolAttributeMap.displayName,
-		oldap.EscapeDNAttributeValue(school.GetDisplayName()),
+	attributeTypeAndValue := ldap.AttributeTypeAndValue{
+		Type:  i.educationConfig.schoolAttributeMap.displayName,
+		Value: school.GetDisplayName(),
+	}
+
+	dn := fmt.Sprintf("%s,%s",
+		attributeTypeAndValue.String(),
 		i.educationConfig.schoolBaseDN,
 	)
 	ar := ldap.NewAddRequest(dn, nil)
@@ -171,13 +174,12 @@ func (i *LDAP) UpdateEducationSchoolOperation(
 // updateDisplayName updates the school OU in the identity backend
 func (i *LDAP) updateDisplayName(ctx context.Context, dn string, providedDisplayName string) error {
 	logger := i.logger.SubloggerWithRequestID(ctx)
-	newDisplayName := fmt.Sprintf(
-		"%s=%s",
-		i.educationConfig.schoolAttributeMap.displayName,
-		providedDisplayName,
-	)
+	attributeTypeAndValue := ldap.AttributeTypeAndValue{
+		Type:  i.educationConfig.schoolAttributeMap.displayName,
+		Value: providedDisplayName,
+	}
 
-	mrdn := ldap.NewModifyDNRequest(dn, newDisplayName, true, "")
+	mrdn := ldap.NewModifyDNRequest(dn, attributeTypeAndValue.String(), true, "")
 	i.logger.Debug().Str("backend", "ldap").
 		Str("dn", mrdn.DN).
 		Str("newrdn", mrdn.NewRDN).
diff --git a/services/graph/pkg/identity/ldap_group.go b/services/graph/pkg/identity/ldap_group.go
index 786235cdc2..778ae5b897 100644
--- a/services/graph/pkg/identity/ldap_group.go
+++ b/services/graph/pkg/identity/ldap_group.go
@@ -11,7 +11,6 @@ import (
 	"github.com/gofrs/uuid"
 	ldapdn "github.com/libregraph/idm/pkg/ldapdn"
 	libregraph "github.com/owncloud/libre-graph-api-go"
-	oldap "github.com/owncloud/ocis/v2/ocis-pkg/ldap"
 	"github.com/owncloud/ocis/v2/services/graph/pkg/service/v0/errorcode"
 	"golang.org/x/exp/slices"
 )
@@ -313,7 +312,11 @@ func (i *LDAP) groupToAddRequest(group libregraph.Group) (*ldap.AddRequest, erro
 }
 
 func (i *LDAP) getGroupLDAPDN(group libregraph.Group) string {
-	return fmt.Sprintf("cn=%s,%s", oldap.EscapeDNAttributeValue(group.GetDisplayName()), i.groupBaseDN)
+	attributeTypeAndValue := ldap.AttributeTypeAndValue{
+		Type:  "cn",
+		Value: group.GetDisplayName(),
+	}
+	return fmt.Sprintf("%s,%s", attributeTypeAndValue.String(), i.groupBaseDN)
 }
 
 func (i *LDAP) groupToLDAPAttrValues(group libregraph.Group) (map[string][]string, error) {