graph: new config option GRAPH_LDAP_GROUP_CREATE_BASE_DN

By setting GRAPH_LDAP_GROUP_CREATE_BASE_DN a distinct subtree can be
configured where new LDAP groups are created. That subtree needs to be
subordinate to GRAPH_LDAP_GROUP_BASE_DN. All groups outside for
GRAPH_LDAP_GROUP_CREATE_BASE_DN are considered read-only and only groups
below that DN can be updated and deleted.

This is introduced for a pretty specific usecase where most groups are managed
in an external source (e.g. a read-only replica of an LDAP tree). But we still
want to allow the local administrator to create groups in a writeable subtree
attached to that replica.

changelog/unreleased/enhancement-ldap-group-create-basedn.md

@@ -0,0 +1,13 @@
+Enhancement: Make the LDAP base DN for new groups configurable
+
+The LDAP backend for the Graph service introduced a new config option for setting the
+Parent DN for new groups created via the `/groups/` endpoint. (`GRAPH_LDAP_GROUP_CREATE_BASE_DN`)
+
+It defaults to the value of `GRAPH_LDAP_GROUP_BASE_DN`. If set to a different value the
+`GRAPH_LDAP_GROUP_CREATE_BASE_DN` needs to be a subordinate DN of `GRAPH_LDAP_GROUP_BASE_DN`.
+
+All existing groups with a DN outside the `GRAPH_LDAP_GROUP_CREATE_BASE_DN` tree will be treated as
+read-only groups. So it is not possible to edit these groups.
+
+https://github.com/owncloud/ocis/pull/5974
+

services/graph/pkg/config/config.go

@@ -69,6 +69,7 @@ type LDAP struct {
 	LdapDisabledUsersGroupDN string `yaml:"ldap_disabled_users_group_dn" env:"LDAP_DISABLED_USERS_GROUP_DN;GRAPH_DISABLED_USERS_GROUP_DN" desc:"The distinguished name of the group to which added users will be classified as disabled when 'disable_user_mechanism' is set to 'group'."`
 
 	GroupBaseDN        string `yaml:"group_base_dn" env:"LDAP_GROUP_BASE_DN;GRAPH_LDAP_GROUP_BASE_DN" desc:"Search base DN for looking up LDAP groups."`
+	GroupCreateBaseDN  string `yaml:"group_create_base_dn" env:"GRAPH_LDAP_GROUP_CREATE_BASE_DN" desc:"Parent DN under which new groups are created. This DN needs to be subordinate to the 'GRAPH_LDAP_GROUP_BASE_DN'. This setting is only relevant when 'GRAPH_LDAP_SERVER_WRITE_ENABLED' is 'true'. It defaults to the value of 'GRAPH_LDAP_GROUP_BASE_DN'. All groups outside of this subtree are treated as readonly groups and cannot be updated."`
 	GroupSearchScope   string `yaml:"group_search_scope" env:"LDAP_GROUP_SCOPE;GRAPH_LDAP_GROUP_SEARCH_SCOPE" desc:"LDAP search scope to use when looking up groups. Supported scopes are 'base', 'one' and 'sub'."`
 	GroupFilter        string `yaml:"group_filter" env:"LDAP_GROUP_FILTER;GRAPH_LDAP_GROUP_FILTER" desc:"LDAP filter to add to the default filters for group searches."`
 	GroupObjectClass   string `yaml:"group_objectclass" env:"LDAP_GROUP_OBJECTCLASS;GRAPH_LDAP_GROUP_OBJECTCLASS" desc:"The object class to use for groups in the default group search filter ('groupOfNames'). "`

services/graph/pkg/config/defaults/defaultconfig.go

@@ -159,6 +159,10 @@ func EnsureDefaults(cfg *config.Config) {
 	if cfg.MachineAuthAPIKey == "" && cfg.Commons != nil && cfg.Commons.MachineAuthAPIKey != "" {
 		cfg.MachineAuthAPIKey = cfg.Commons.MachineAuthAPIKey
 	}
+
+	if cfg.Identity.LDAP.GroupCreateBaseDN == "" {
+		cfg.Identity.LDAP.GroupCreateBaseDN = cfg.Identity.LDAP.GroupBaseDN
+	}
 }
 
 // Sanitize sanitized the configuration

services/graph/pkg/config/parser/parse.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 
+	"github.com/go-ldap/ldap/v3"
 	ociscfg "github.com/owncloud/ocis/v2/ocis-pkg/config"
 	defaults2 "github.com/owncloud/ocis/v2/ocis-pkg/config/defaults"
 	"github.com/owncloud/ocis/v2/ocis-pkg/shared"
@@ -40,8 +41,10 @@ func Validate(cfg *config.Config) error {
 		return shared.MissingJWTTokenError(cfg.Service.Name)
 	}
 
-	if cfg.Identity.Backend == "ldap" && cfg.Identity.LDAP.BindPassword == "" {
-		return shared.MissingLDAPBindPassword(cfg.Service.Name)
+	if cfg.Identity.Backend == "ldap" {
+		if err := validateLDAPSettings(cfg); err != nil {
+			return err
+		}
 	}
 
 	if cfg.Application.ID == "" {
@@ -64,3 +67,26 @@ func Validate(cfg *config.Config) error {
 
 	return nil
 }
+
+func validateLDAPSettings(cfg *config.Config) error {
+	if cfg.Identity.LDAP.BindPassword == "" {
+		return shared.MissingLDAPBindPassword(cfg.Service.Name)
+	}
+
+	// ensure that "GroupBaseDN" is below "GroupBaseDN"
+	if cfg.Identity.LDAP.WriteEnabled && cfg.Identity.LDAP.GroupCreateBaseDN != cfg.Identity.LDAP.GroupBaseDN {
+		baseDN, err := ldap.ParseDN(cfg.Identity.LDAP.GroupBaseDN)
+		if err != nil {
+			return fmt.Errorf("Unable to parse the LDAP Group Base DN '%s': %w ", cfg.Identity.LDAP.GroupBaseDN, err)
+		}
+		createBaseDN, err := ldap.ParseDN(cfg.Identity.LDAP.GroupCreateBaseDN)
+		if err != nil {
+			return fmt.Errorf("Unable to parse the LDAP Group Create Base DN '%s': %w ", cfg.Identity.LDAP.GroupCreateBaseDN, err)
+		}
+
+		if !baseDN.AncestorOfFold(createBaseDN) {
+			return fmt.Errorf("The LDAP Group Create Base DN (%s) must be subordinate to the LDAP Group Base DN (%s)", cfg.Identity.LDAP.GroupCreateBaseDN, cfg.Identity.LDAP.GroupBaseDN)
+		}
+	}
+	return nil
+}

services/graph/pkg/identity/ldap.go

@@ -58,6 +58,7 @@ type LDAP struct {
 	localUserDisableGroupDN string
 
 	groupBaseDN       string
+	groupCreateBaseDN string
 	groupFilter       string
 	groupObjectClass  string
 	groupScope        int
@@ -148,6 +149,7 @@ func NewLDAPBackend(lc ldap.Client, config config.LDAP, logger *log.Logger) (*LD
 		userScope:               userScope,
 		userAttributeMap:        uam,
 		groupBaseDN:             config.GroupBaseDN,
+		groupCreateBaseDN:       config.GroupCreateBaseDN,
 		groupFilter:             config.GroupFilter,
 		groupObjectClass:        config.GroupObjectClass,
 		groupScope:              groupScope,

services/graph/pkg/identity/ldap_group.go

@@ -204,10 +204,16 @@ func (i *LDAP) DeleteGroup(ctx context.Context, id string) error {
 	if !i.writeEnabled {
 		return errorcode.New(errorcode.NotAllowed, "server is configured read-only")
 	}
+
 	e, err := i.getLDAPGroupByID(id, false)
 	if err != nil {
 		return err
 	}
+
+	if i.isLDAPGroupReadOnly(e) {
+		return errorcode.New(errorcode.NotAllowed, "group is read-only")
+	}
+
 	dr := ldap.DelRequest{DN: e.DN}
 	if err = i.conn.Del(&dr); err != nil {
 		return err
@@ -219,12 +225,19 @@ func (i *LDAP) DeleteGroup(ctx context.Context, id string) error {
 func (i *LDAP) UpdateGroupName(ctx context.Context, groupID string, groupName string) error {
 	logger := i.logger.SubloggerWithRequestID(ctx)
 	logger.Debug().Str("backend", "ldap").Msg("AddMembersToGroup")
+	if !i.writeEnabled {
+		return errorcode.New(errorcode.NotAllowed, "server is configured read-only")
+	}
 
 	ge, err := i.getLDAPGroupByID(groupID, true)
 	if err != nil {
 		return err
 	}
 
+	if i.isLDAPGroupReadOnly(ge) {
+		return errorcode.New(errorcode.NotAllowed, "group is read-only")
+	}
+
 	if ge.GetEqualFoldAttributeValue(i.groupAttributeMap.name) == groupName {
 		return nil
 	}
@@ -258,11 +271,18 @@ func (i *LDAP) UpdateGroupName(ctx context.Context, groupID string, groupName st
 func (i *LDAP) AddMembersToGroup(ctx context.Context, groupID string, memberIDs []string) error {
 	logger := i.logger.SubloggerWithRequestID(ctx)
 	logger.Debug().Str("backend", "ldap").Msg("AddMembersToGroup")
+	if !i.writeEnabled {
+		return errorcode.New(errorcode.NotAllowed, "server is configured read-only")
+	}
 	ge, err := i.getLDAPGroupByNameOrID(groupID, true)
 	if err != nil {
 		return err
 	}
 
+	if i.isLDAPGroupReadOnly(ge) {
+		return errorcode.New(errorcode.NotAllowed, "group is read-only")
+	}
+
 	mr := ldap.ModifyRequest{DN: ge.DN}
 	// Handle empty groups (using the empty member attribute)
 	current := ge.GetEqualFoldAttributeValues(i.groupAttributeMap.member)
@@ -326,11 +346,20 @@ func (i *LDAP) AddMembersToGroup(ctx context.Context, groupID string, memberIDs
 func (i *LDAP) RemoveMemberFromGroup(ctx context.Context, groupID string, memberID string) error {
 	logger := i.logger.SubloggerWithRequestID(ctx)
 	logger.Debug().Str("backend", "ldap").Msg("RemoveMemberFromGroup")
+	if !i.writeEnabled {
+		return errorcode.New(errorcode.NotAllowed, "server is configured read-only")
+	}
+
 	ge, err := i.getLDAPGroupByID(groupID, true)
 	if err != nil {
 		logger.Debug().Str("backend", "ldap").Str("groupID", groupID).Msg("Error looking up group")
 		return err
 	}
+
+	if i.isLDAPGroupReadOnly(ge) {
+		return errorcode.New(errorcode.NotAllowed, "group is read-only")
+	}
+
 	me, err := i.getLDAPUserByID(memberID)
 	if err != nil {
 		logger.Debug().Str("backend", "ldap").Str("memberID", memberID).Msg("Error looking up group member")
@@ -345,7 +374,7 @@ func (i *LDAP) RemoveMemberFromGroup(ctx context.Context, groupID string, member
 }
 
 func (i *LDAP) groupToAddRequest(group libregraph.Group) (*ldap.AddRequest, error) {
-	ar := ldap.NewAddRequest(i.getGroupLDAPDN(group), nil)
+	ar := ldap.NewAddRequest(i.getGroupCreateLDAPDN(group), nil)
 
 	attrMap, err := i.groupToLDAPAttrValues(group)
 	if err != nil {
@@ -357,12 +386,12 @@ func (i *LDAP) groupToAddRequest(group libregraph.Group) (*ldap.AddRequest, erro
 	return ar, nil
 }
 
-func (i *LDAP) getGroupLDAPDN(group libregraph.Group) string {
+func (i *LDAP) getGroupCreateLDAPDN(group libregraph.Group) string {
 	attributeTypeAndValue := ldap.AttributeTypeAndValue{
 		Type:  "cn",
 		Value: group.GetDisplayName(),
 	}
-	return fmt.Sprintf("%s,%s", attributeTypeAndValue.String(), i.groupBaseDN)
+	return fmt.Sprintf("%s,%s", attributeTypeAndValue.String(), i.groupCreateBaseDN)
 }
 
 func (i *LDAP) groupToLDAPAttrValues(group libregraph.Group) (map[string][]string, error) {
@@ -482,17 +511,43 @@ func (i *LDAP) getGroupsForUser(dn string) ([]*ldap.Entry, error) {
 func (i *LDAP) createGroupModelFromLDAP(e *ldap.Entry) *libregraph.Group {
 	name := e.GetEqualFoldAttributeValue(i.groupAttributeMap.name)
 	id := e.GetEqualFoldAttributeValue(i.groupAttributeMap.id)
+	groupTypes := []string{}
+
+	if i.isLDAPGroupReadOnly(e) {
+		groupTypes = []string{"ReadOnly"}
+	}
 
 	if id != "" && name != "" {
 		return &libregraph.Group{
 			DisplayName: &name,
 			Id:          &id,
+			GroupTypes:  groupTypes,
 		}
 	}
 	i.logger.Warn().Str("dn", e.DN).Msg("Group is missing name or id")
 	return nil
 }
 
+func (i *LDAP) isLDAPGroupReadOnly(e *ldap.Entry) bool {
+	if !i.writeEnabled {
+		return true
+	}
+
+	groupDN, err := ldap.ParseDN(e.DN)
+	if err != nil {
+		i.logger.Warn().Err(err).Str("dn", e.DN).Msg("Failed to parse DN")
+		return false
+	}
+
+	baseDN, err := ldap.ParseDN(i.groupCreateBaseDN)
+	if err != nil {
+		i.logger.Warn().Err(err).Str("dn", i.groupCreateBaseDN).Msg("Failed to parse DN")
+		return false
+	}
+
+	return !baseDN.AncestorOfFold(groupDN)
+}
+
 func (i *LDAP) groupsFromLDAPEntries(e []*ldap.Entry) []libregraph.Group {
 	groups := make([]libregraph.Group, 0, len(e))
 	for _, g := range e {

services/graph/pkg/identity/ldap_group_test.go

@@ -21,22 +21,42 @@ var groupEntry = ldap.NewEntry("cn=group",
 			"uid=invalid,ou=people,dc=test",
 		},
 	})
+
 var invalidGroupEntry = ldap.NewEntry("cn=invalid",
 	map[string][]string{
 		"cn": {"invalid"},
 	})
 
+var queryParamExpand = url.Values{
+	"$expand": []string{"members"},
+}
+
+var queryParamSelect = url.Values{
+	"$select": []string{"members"},
+}
+
+var groupLookupSearchRequest = &ldap.SearchRequest{
+	BaseDN:     "ou=groups,dc=test",
+	Scope:      2,
+	SizeLimit:  1,
+	Filter:     "(&(objectClass=groupOfNames)(|(cn=group)(entryUUID=group)))",
+	Attributes: []string{"cn", "entryUUID", "member"},
+	Controls:   []ldap.Control(nil),
+}
+
+var groupListSeachRequest = &ldap.SearchRequest{
+	BaseDN:     "ou=groups,dc=test",
+	Scope:      2,
+	Filter:     "(&(objectClass=groupOfNames))",
+	Attributes: []string{"cn", "entryUUID", "member"},
+	Controls:   []ldap.Control(nil),
+}
+
 func TestGetGroup(t *testing.T) {
 	// Mock a Sizelimit Error
 	lm := &mocks.Client{}
 	lm.On("Search", mock.Anything).Return(nil, ldap.NewError(ldap.LDAPResultSizeLimitExceeded, errors.New("mock")))
 
-	queryParamExpand := url.Values{
-		"$expand": []string{"members"},
-	}
-	queryParamSelect := url.Values{
-		"$select": []string{"members"},
-	}
 	b, _ := getMockedBackend(lm, lconfig, &logger)
 	_, err := b.GetGroup(context.Background(), "group", nil)
 	if err == nil || err.Error() != "itemNotFound" {
@@ -89,14 +109,6 @@ func TestGetGroup(t *testing.T) {
 
 	// Mock a valid	Search Result
 	lm = &mocks.Client{}
-	sr1 := &ldap.SearchRequest{
-		BaseDN:     "ou=groups,dc=test",
-		Scope:      2,
-		SizeLimit:  1,
-		Filter:     "(&(objectClass=groupOfNames)(|(cn=group)(entryUUID=group)))",
-		Attributes: []string{"cn", "entryUUID", "member"},
-		Controls:   []ldap.Control(nil),
-	}
 	sr2 := &ldap.SearchRequest{
 		BaseDN:     "uid=user,ou=people,dc=test",
 		SizeLimit:  1,
@@ -112,7 +124,7 @@ func TestGetGroup(t *testing.T) {
 		Controls:   []ldap.Control(nil),
 	}
 
-	lm.On("Search", sr1).Return(&ldap.SearchResult{Entries: []*ldap.Entry{groupEntry}}, nil)
+	lm.On("Search", groupLookupSearchRequest).Return(&ldap.SearchResult{Entries: []*ldap.Entry{groupEntry}}, nil)
 	lm.On("Search", sr2).Return(&ldap.SearchResult{Entries: []*ldap.Entry{userEntry}}, nil)
 	lm.On("Search", sr3).Return(&ldap.SearchResult{Entries: []*ldap.Entry{invalidUserEntry}}, nil)
 	b, _ = getMockedBackend(lm, lconfig, &logger)
@@ -146,13 +158,80 @@ func TestGetGroup(t *testing.T) {
 	}
 }
 
+func TestGetGroupReadOnlyBackend(t *testing.T) {
+	readOnlyConfig := lconfig
+	readOnlyConfig.WriteEnabled = false
+
+	lm := &mocks.Client{}
+	lm.On("Search", groupLookupSearchRequest).Return(&ldap.SearchResult{Entries: []*ldap.Entry{groupEntry}}, nil)
+	b, _ := getMockedBackend(lm, readOnlyConfig, &logger)
+	g, err := b.GetGroup(context.Background(), "group", url.Values{})
+	switch {
+	case err != nil:
+		t.Errorf("Expected GetGroup to succeed. Got %s", err.Error())
+	case g.GetId() != groupEntry.GetEqualFoldAttributeValue(b.groupAttributeMap.id):
+		t.Errorf("Expected GetGroup to return a valid group")
+	}
+	types := g.GetGroupTypes()
+	switch {
+	case len(types) == 0:
+		t.Errorf("No groupTypes attribute on readonly Group")
+	case len(types) > 1:
+		t.Errorf("Expected a single groupTypes value on readonly Group")
+	case types[0] != "ReadOnly":
+		t.Errorf("Expected a groupTypes 'ReadOnly' on readonly Group")
+	}
+}
+func TestGetGroupReadOnlySubtree(t *testing.T) {
+	readOnlyTreeConfig := lconfig
+	readOnlyTreeConfig.GroupCreateBaseDN = "ou=write,ou=groups,dc=test"
+	var writeGroupEntry = ldap.NewEntry("cn=group,ou=write,ou=groups,dc=test",
+		map[string][]string{
+			"cn":        {"group"},
+			"entryuuid": {"abcd-defg"},
+			"member": {
+				"uid=user,ou=people,dc=test",
+				"uid=invalid,ou=people,dc=test",
+			},
+		})
+
+	lm := &mocks.Client{}
+	lm.On("Search", groupLookupSearchRequest).Return(&ldap.SearchResult{Entries: []*ldap.Entry{groupEntry}}, nil)
+	b, _ := getMockedBackend(lm, readOnlyTreeConfig, &logger)
+	g, err := b.GetGroup(context.Background(), "group", url.Values{})
+	switch {
+	case err != nil:
+		t.Errorf("Expected GetGroup to succeed. Got %s", err.Error())
+	case g.GetId() != groupEntry.GetEqualFoldAttributeValue(b.groupAttributeMap.id):
+		t.Errorf("Expected GetGroup to return a valid group")
+	}
+	types := g.GetGroupTypes()
+	switch {
+	case len(types) == 0:
+		t.Errorf("No groupTypes attribute on readonly Group")
+	case len(types) > 1:
+		t.Errorf("Expected a single groupTypes value on readonly Group")
+	case types[0] != "ReadOnly":
+		t.Errorf("Expected a groupTypes 'ReadOnly' on readonly Group")
+	}
+
+	lm = &mocks.Client{}
+	lm.On("Search", groupLookupSearchRequest).Return(&ldap.SearchResult{Entries: []*ldap.Entry{writeGroupEntry}}, nil)
+	b, _ = getMockedBackend(lm, readOnlyTreeConfig, &logger)
+	g, err = b.GetGroup(context.Background(), "group", url.Values{})
+	switch {
+	case err != nil:
+		t.Errorf("Expected GetGroup to succeed. Got %s", err.Error())
+	case g.GetId() != groupEntry.GetEqualFoldAttributeValue(b.groupAttributeMap.id):
+		t.Errorf("Expected GetGroup to return a valid group")
+	}
+	types = g.GetGroupTypes()
+	if len(types) != 0 {
+		t.Errorf("No groupTypes attribute expected on writeable Group")
+	}
+}
+
 func TestGetGroups(t *testing.T) {
-	queryParamExpand := url.Values{
-		"$expand": []string{"members"},
-	}
-	queryParamSelect := url.Values{
-		"$select": []string{"members"},
-	}
 	lm := &mocks.Client{}
 	lm.On("Search", mock.Anything).Return(nil, ldap.NewError(ldap.LDAPResultOperationsError, errors.New("mock")))
 	b, _ := getMockedBackend(lm, lconfig, &logger)
@@ -185,13 +264,6 @@ func TestGetGroups(t *testing.T) {
 
 	// Mock a valid	Search Result with expanded group members
 	lm = &mocks.Client{}
-	sr1 := &ldap.SearchRequest{
-		BaseDN:     "ou=groups,dc=test",
-		Scope:      2,
-		Filter:     "(&(objectClass=groupOfNames))",
-		Attributes: []string{"cn", "entryUUID", "member"},
-		Controls:   []ldap.Control(nil),
-	}
 	sr2 := &ldap.SearchRequest{
 		BaseDN:     "uid=user,ou=people,dc=test",
 		SizeLimit:  1,
@@ -208,7 +280,7 @@ func TestGetGroups(t *testing.T) {
 	}
 
 	for _, param := range []url.Values{queryParamSelect, queryParamExpand} {
-		lm.On("Search", sr1).Return(&ldap.SearchResult{Entries: []*ldap.Entry{groupEntry}}, nil)
+		lm.On("Search", groupListSeachRequest).Return(&ldap.SearchResult{Entries: []*ldap.Entry{groupEntry}}, nil)
 		lm.On("Search", sr2).Return(&ldap.SearchResult{Entries: []*ldap.Entry{userEntry}}, nil)
 		lm.On("Search", sr3).Return(&ldap.SearchResult{Entries: []*ldap.Entry{invalidUserEntry}}, nil)
 		b, _ = getMockedBackend(lm, lconfig, &logger)

services/graph/pkg/service/v0/errorcode/errorcode.go

@@ -106,6 +106,8 @@ func (e Error) Render(w http.ResponseWriter, r *http.Request) {
 		status = http.StatusNotFound
 	case NameAlreadyExists:
 		status = http.StatusConflict
+	case NotAllowed:
+		status = http.StatusForbidden
 	default:
 		status = http.StatusInternalServerError
 	}

services/graph/pkg/service/v0/groups.go

@@ -82,8 +82,13 @@ func (g Graph) PostGroup(w http.ResponseWriter, r *http.Request) {
 	}
 
 	if grp, err = g.identityBackend.CreateGroup(r.Context(), *grp); err != nil {
-		logger.Debug().Interface("group", grp).Msg("could not create group: backend error")
-		errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
+		var errcode errorcode.Error
+		if errors.As(err, &errcode) {
+			errcode.Render(w, r)
+		} else {
+			logger.Debug().Interface("group", grp).Msg("could not create group: backend error")
+			errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
+		}
 		return
 	}