From ee57f01bd792e37504f05d0b2af64247814c1c84 Mon Sep 17 00:00:00 2001 From: Abhidas747 Date: Wed, 17 Jun 2026 19:05:06 +0530 Subject: [PATCH 1/8] [DATA-6657] LDAP nested query support Signed-off-by: Abhidas747 --- api/v1alpha1/group_types.go | 33 ++- api/v1alpha1/zz_generated.deepcopy.go | 81 +++++ .../operator.dataverse.redhat.com_groups.yaml | 153 +++++++++- .../v1alpha1_group_querybased_nested.yaml | 38 +++ internal/controller/group_controller.go | 172 +++++++++-- .../group_controller_manager_query_test.go | 254 ++++++++++++++++ pkg/clients/ldap/query.go | 90 +++++- pkg/clients/ldap/query_test.go | 280 +++++++++++++++++- 8 files changed, 1059 insertions(+), 42 deletions(-) create mode 100644 config/samples/v1alpha1_group_querybased_nested.yaml create mode 100644 internal/controller/group_controller_manager_query_test.go diff --git a/api/v1alpha1/group_types.go b/api/v1alpha1/group_types.go index fef92212..15ff092b 100644 --- a/api/v1alpha1/group_types.go +++ b/api/v1alpha1/group_types.go @@ -44,11 +44,42 @@ type LDAPFilter struct { Value string `json:"value"` } +// +kubebuilder:validation:XValidation:rule="has(self.filters) || has(self.queries)",message="at least one of filters or queries must be specified" type LDAPQuery struct { + // +kubebuilder:validation:Enum=and;or + Operator string `json:"operator"` + // +optional + Filters []LDAPFilter `json:"filters,omitempty"` + // +optional + Queries []LDAPSubQuery `json:"queries,omitempty"` + // +optional + Options *LDAPOptions `json:"options,omitempty"` +} + +// +kubebuilder:validation:XValidation:rule="has(self.filters) || has(self.queries)",message="at least one of filters or queries must be specified" +type LDAPSubQuery struct { + // +kubebuilder:validation:Enum=and;or + Operator string `json:"operator"` + // +optional + Filters []LDAPFilter `json:"filters,omitempty"` + // +optional + Queries []LDAPLeafQuery `json:"queries,omitempty"` +} + +// +kubebuilder:validation:XValidation:rule="has(self.filters) || has(self.queries)",message="at least one of filters or queries must be specified" +type LDAPLeafQuery struct { + // +kubebuilder:validation:Enum=and;or + Operator string `json:"operator"` + // +optional + Filters []LDAPFilter `json:"filters,omitempty"` + // +optional + Queries []LDAPLeafSubQuery `json:"queries,omitempty"` +} + +type LDAPLeafSubQuery struct { // +kubebuilder:validation:Enum=and;or Operator string `json:"operator"` Filters []LDAPFilter `json:"filters"` - Options *LDAPOptions `json:"options,omitempty"` } type LDAPOptions struct { diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index cc7de36e..5e003bfc 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -209,6 +209,53 @@ func (in *LDAPFilter) DeepCopy() *LDAPFilter { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *LDAPLeafQuery) DeepCopyInto(out *LDAPLeafQuery) { + *out = *in + if in.Filters != nil { + in, out := &in.Filters, &out.Filters + *out = make([]LDAPFilter, len(*in)) + copy(*out, *in) + } + if in.Queries != nil { + in, out := &in.Queries, &out.Queries + *out = make([]LDAPLeafSubQuery, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LDAPLeafQuery. +func (in *LDAPLeafQuery) DeepCopy() *LDAPLeafQuery { + if in == nil { + return nil + } + out := new(LDAPLeafQuery) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *LDAPLeafSubQuery) DeepCopyInto(out *LDAPLeafSubQuery) { + *out = *in + if in.Filters != nil { + in, out := &in.Filters, &out.Filters + *out = make([]LDAPFilter, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LDAPLeafSubQuery. +func (in *LDAPLeafSubQuery) DeepCopy() *LDAPLeafSubQuery { + if in == nil { + return nil + } + out := new(LDAPLeafSubQuery) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *LDAPOptions) DeepCopyInto(out *LDAPOptions) { *out = *in @@ -232,6 +279,13 @@ func (in *LDAPQuery) DeepCopyInto(out *LDAPQuery) { *out = make([]LDAPFilter, len(*in)) copy(*out, *in) } + if in.Queries != nil { + in, out := &in.Queries, &out.Queries + *out = make([]LDAPSubQuery, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } if in.Options != nil { in, out := &in.Options, &out.Options *out = new(LDAPOptions) @@ -249,6 +303,33 @@ func (in *LDAPQuery) DeepCopy() *LDAPQuery { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *LDAPSubQuery) DeepCopyInto(out *LDAPSubQuery) { + *out = *in + if in.Filters != nil { + in, out := &in.Filters, &out.Filters + *out = make([]LDAPFilter, len(*in)) + copy(*out, *in) + } + if in.Queries != nil { + in, out := &in.Queries, &out.Queries + *out = make([]LDAPLeafQuery, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LDAPSubQuery. +func (in *LDAPSubQuery) DeepCopy() *LDAPSubQuery { + if in == nil { + return nil + } + out := new(LDAPSubQuery) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Members) DeepCopyInto(out *Members) { *out = *in diff --git a/config/crd/bases/operator.dataverse.redhat.com_groups.yaml b/config/crd/bases/operator.dataverse.redhat.com_groups.yaml index 84413419..7528246d 100644 --- a/config/crd/bases/operator.dataverse.redhat.com_groups.yaml +++ b/config/crd/bases/operator.dataverse.redhat.com_groups.yaml @@ -139,10 +139,161 @@ spec: include_manager: type: boolean type: object + queries: + items: + properties: + filters: + items: + properties: + criteria: + enum: + - equals + - contains + - not + type: string + key: + enum: + - givenName + - displayName + - rhatJobTitle + - title + - employeeType + - manager + - rhatCostCenter + - rhatCostCenterDesc + - rhatGeo + - co + - st + - rhatLocation + - rhatOfficeLocation + - rhatOfficeFloor + - roomNumber + type: string + value: + type: string + required: + - criteria + - key + - value + type: object + type: array + operator: + enum: + - and + - or + type: string + queries: + items: + properties: + filters: + items: + properties: + criteria: + enum: + - equals + - contains + - not + type: string + key: + enum: + - givenName + - displayName + - rhatJobTitle + - title + - employeeType + - manager + - rhatCostCenter + - rhatCostCenterDesc + - rhatGeo + - co + - st + - rhatLocation + - rhatOfficeLocation + - rhatOfficeFloor + - roomNumber + type: string + value: + type: string + required: + - criteria + - key + - value + type: object + type: array + operator: + enum: + - and + - or + type: string + queries: + items: + properties: + filters: + items: + properties: + criteria: + enum: + - equals + - contains + - not + type: string + key: + enum: + - givenName + - displayName + - rhatJobTitle + - title + - employeeType + - manager + - rhatCostCenter + - rhatCostCenterDesc + - rhatGeo + - co + - st + - rhatLocation + - rhatOfficeLocation + - rhatOfficeFloor + - roomNumber + type: string + value: + type: string + required: + - criteria + - key + - value + type: object + type: array + operator: + enum: + - and + - or + type: string + required: + - filters + - operator + type: object + type: array + required: + - operator + type: object + x-kubernetes-validations: + - message: at least one of filters or queries must + be specified + rule: has(self.filters) || has(self.queries) + type: array + required: + - operator + type: object + x-kubernetes-validations: + - message: at least one of filters or queries must be specified + rule: has(self.filters) || has(self.queries) + type: array required: - - filters - operator type: object + x-kubernetes-validations: + - message: at least one of filters or queries must be specified + rule: has(self.filters) || has(self.queries) users: items: type: string diff --git a/config/samples/v1alpha1_group_querybased_nested.yaml b/config/samples/v1alpha1_group_querybased_nested.yaml new file mode 100644 index 00000000..4a1775e0 --- /dev/null +++ b/config/samples/v1alpha1_group_querybased_nested.yaml @@ -0,0 +1,38 @@ +apiVersion: operator.dataverse.redhat.com/v1alpha1 +kind: Group +metadata: + labels: + app.kubernetes.io/name: usernaut + app.kubernetes.io/managed-by: kustomize + name: test-nested-query + namespace: usernaut +spec: + group_name: dataverse-consumer-test-nested + members: + ldap_query: + options: + include_indirect_reports: true + include_manager: true + operator: and + filters: + - key: employeeType + criteria: not + value: "external employee" + queries: + - operator: or + filters: + - key: title + criteria: contains + value: engineer + - key: title + criteria: contains + value: developer + - operator: or + filters: + - key: manager + criteria: equals + value: mgrAlpha + - key: manager + criteria: equals + value: mgrBeta + backends: [] diff --git a/internal/controller/group_controller.go b/internal/controller/group_controller.go index d972a619..dcdcb09a 100644 --- a/internal/controller/group_controller.go +++ b/internal/controller/group_controller.go @@ -264,13 +264,7 @@ func (r *GroupReconciler) fetchQueryMembers(ctx context.Context, query *usernaut log.WithField("query_members_count", len(queryMembers)).Info("query members fetched successfully") - hasManagerFilter := false - for _, filter := range query.Filters { - if strings.EqualFold(strings.TrimSpace(filter.Key), "manager") { - hasManagerFilter = true - break - } - } + hasManagerFilter := queryHasManagerFilter(query) // Manager filter present but indirect reports disabled: return only direct reports of the manager in the query (no recursion). if hasManagerFilter && !includeIndirectReports { @@ -304,18 +298,8 @@ func (r *GroupReconciler) fetchQueryMembers(ctx context.Context, query *usernaut } visited[member] = struct{}{} - nestedQuery.Filters = make([]usernautdevv1alpha1.LDAPFilter, 0, len(query.Filters)) - for _, filter := range query.Filters { - value := filter.Value - if strings.EqualFold(strings.TrimSpace(filter.Key), "manager") { - value = member - } - nestedQuery.Filters = append(nestedQuery.Filters, usernautdevv1alpha1.LDAPFilter{ - Key: filter.Key, - Criteria: filter.Criteria, - Value: value, - }) - } + nestedQuery.Filters = replaceManagerInFilters(query.Filters, member) + nestedQuery.Queries = replaceManagerInSubQueries(query.Queries, member) nestedQueryMembers, err := r.fetchQueryMembers(ctx, &nestedQuery, includeIndirectReports, visited) if err != nil { log.WithError(err).WithField("manager", member).Error("error fetching indirect reports") @@ -332,18 +316,144 @@ func (r *GroupReconciler) fetchQueryMembers(ctx context.Context, query *usernaut return r.deduplicateMembers(queryMembers), nil } +// replaceManagerInFilters returns a copy of filters where every manager filter value +// is replaced with managerUID. Duplicate manager filters with the same criteria and +// value after replacement are collapsed to a single entry. +func replaceManagerInFilters(filters []usernautdevv1alpha1.LDAPFilter, managerUID string) []usernautdevv1alpha1.LDAPFilter { + result := make([]usernautdevv1alpha1.LDAPFilter, 0, len(filters)) + seenManagers := make(map[string]struct{}) + for _, filter := range filters { + value := filter.Value + if strings.EqualFold(strings.TrimSpace(filter.Key), "manager") { + value = managerUID + dedupeKey := strings.ToLower(strings.TrimSpace(filter.Criteria)) + "|" + value + if _, ok := seenManagers[dedupeKey]; ok { + continue + } + seenManagers[dedupeKey] = struct{}{} + } + result = append(result, usernautdevv1alpha1.LDAPFilter{ + Key: filter.Key, + Criteria: filter.Criteria, + Value: value, + }) + } + return result +} + +// replaceManagerInSubQueries rewrites manager filter values at all nested levels. +func replaceManagerInSubQueries(queries []usernautdevv1alpha1.LDAPSubQuery, managerUID string) []usernautdevv1alpha1.LDAPSubQuery { + if len(queries) == 0 { + return nil + } + result := make([]usernautdevv1alpha1.LDAPSubQuery, 0, len(queries)) + for _, q := range queries { + result = append(result, usernautdevv1alpha1.LDAPSubQuery{ + Operator: q.Operator, + Filters: replaceManagerInFilters(q.Filters, managerUID), + Queries: replaceManagerInLeafQueries(q.Queries, managerUID), + }) + } + return result +} + +func replaceManagerInLeafQueries(queries []usernautdevv1alpha1.LDAPLeafQuery, managerUID string) []usernautdevv1alpha1.LDAPLeafQuery { + if len(queries) == 0 { + return nil + } + result := make([]usernautdevv1alpha1.LDAPLeafQuery, 0, len(queries)) + for _, q := range queries { + result = append(result, usernautdevv1alpha1.LDAPLeafQuery{ + Operator: q.Operator, + Filters: replaceManagerInFilters(q.Filters, managerUID), + Queries: replaceManagerInLeafSubQueries(q.Queries, managerUID), + }) + } + return result +} + +func replaceManagerInLeafSubQueries(queries []usernautdevv1alpha1.LDAPLeafSubQuery, managerUID string) []usernautdevv1alpha1.LDAPLeafSubQuery { + if len(queries) == 0 { + return nil + } + result := make([]usernautdevv1alpha1.LDAPLeafSubQuery, 0, len(queries)) + for _, q := range queries { + result = append(result, usernautdevv1alpha1.LDAPLeafSubQuery{ + Operator: q.Operator, + Filters: replaceManagerInFilters(q.Filters, managerUID), + }) + } + return result +} + +func filtersHaveManager(filters []usernautdevv1alpha1.LDAPFilter) bool { + for _, filter := range filters { + if strings.EqualFold(strings.TrimSpace(filter.Key), "manager") { + return true + } + } + return false +} + +// queryHasManagerFilter reports whether a manager filter exists anywhere in the query tree. +func queryHasManagerFilter(query *usernautdevv1alpha1.LDAPQuery) bool { + if query == nil { + return false + } + if filtersHaveManager(query.Filters) { + return true + } + for i := range query.Queries { + if subQueryHasManager(&query.Queries[i]) { + return true + } + } + return false +} + +func subQueryHasManager(query *usernautdevv1alpha1.LDAPSubQuery) bool { + if filtersHaveManager(query.Filters) { + return true + } + for i := range query.Queries { + if leafQueryHasManager(&query.Queries[i]) { + return true + } + } + return false +} + +func leafQueryHasManager(query *usernautdevv1alpha1.LDAPLeafQuery) bool { + if filtersHaveManager(query.Filters) { + return true + } + for i := range query.Queries { + if filtersHaveManager(query.Queries[i].Filters) { + return true + } + } + return false +} + +// extractManagerUIDsFromQuery returns unique manager UIDs referenced anywhere in the query tree. func extractManagerUIDsFromQuery(query *usernautdevv1alpha1.LDAPQuery) []string { if query == nil { return nil } - - managerUIDs := make([]string, 0) seen := make(map[string]struct{}) - for _, filter := range query.Filters { + var result []string + collectManagerUIDsFromFilters(query.Filters, seen, &result) + for i := range query.Queries { + collectSubQueryManagerUIDs(&query.Queries[i], seen, &result) + } + return result +} + +func collectManagerUIDsFromFilters(filters []usernautdevv1alpha1.LDAPFilter, seen map[string]struct{}, result *[]string) { + for _, filter := range filters { if !strings.EqualFold(strings.TrimSpace(filter.Key), "manager") { continue } - uid := filter.Value if uid == "" { continue @@ -352,10 +462,22 @@ func extractManagerUIDsFromQuery(query *usernautdevv1alpha1.LDAPQuery) []string continue } seen[uid] = struct{}{} - managerUIDs = append(managerUIDs, uid) + *result = append(*result, uid) + } +} + +func collectSubQueryManagerUIDs(query *usernautdevv1alpha1.LDAPSubQuery, seen map[string]struct{}, result *[]string) { + collectManagerUIDsFromFilters(query.Filters, seen, result) + for i := range query.Queries { + collectLeafQueryManagerUIDs(&query.Queries[i], seen, result) } +} - return managerUIDs +func collectLeafQueryManagerUIDs(query *usernautdevv1alpha1.LDAPLeafQuery, seen map[string]struct{}, result *[]string) { + collectManagerUIDsFromFilters(query.Filters, seen, result) + for i := range query.Queries { + collectManagerUIDsFromFilters(query.Queries[i].Filters, seen, result) + } } // fetchLDAPData fetches LDAP data for all unique members and populates allLdapUserData. diff --git a/internal/controller/group_controller_manager_query_test.go b/internal/controller/group_controller_manager_query_test.go new file mode 100644 index 00000000..2b75973f --- /dev/null +++ b/internal/controller/group_controller_manager_query_test.go @@ -0,0 +1,254 @@ +package controller + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + usernautdevv1alpha1 "github.com/redhat-data-and-ai/usernaut/api/v1alpha1" +) + +func TestReplaceManagerInFilters(t *testing.T) { + t.Parallel() + + filters := []usernautdevv1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, + {Key: "title", Criteria: "contains", Value: "engineer"}, + } + + got := replaceManagerInFilters(filters, "newMgr") + + require.Len(t, got, 2) + assert.Equal(t, "newMgr", got[0].Value) + assert.Equal(t, "engineer", got[1].Value) + assert.Equal(t, "title", got[1].Key) +} + +func TestReplaceManagerInFilters_DeduplicatesMatchingManagers(t *testing.T) { + t.Parallel() + + filters := []usernautdevv1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, + {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + } + + got := replaceManagerInFilters(filters, "newMgr") + + require.Len(t, got, 1) + assert.Equal(t, "manager", got[0].Key) + assert.Equal(t, "equals", got[0].Criteria) + assert.Equal(t, "newMgr", got[0].Value) +} + +func TestReplaceManagerInFilters_PreservesDistinctManagerCriteria(t *testing.T) { + t.Parallel() + + filters := []usernautdevv1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, + {Key: "manager", Criteria: "contains", Value: "mgrAlpha"}, + } + + got := replaceManagerInFilters(filters, "newMgr") + + require.Len(t, got, 2) + assert.Equal(t, "equals", got[0].Criteria) + assert.Equal(t, "contains", got[1].Criteria) + assert.Equal(t, "newMgr", got[0].Value) + assert.Equal(t, "newMgr", got[1].Value) +} + +func TestReplaceManagerInSubQueries(t *testing.T) { + t.Parallel() + + queries := []usernautdevv1alpha1.LDAPSubQuery{ + { + Operator: "or", + Filters: []usernautdevv1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, + {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + }, + Queries: []usernautdevv1alpha1.LDAPLeafQuery{ + { + Operator: "and", + Filters: []usernautdevv1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrGamma"}, + }, + Queries: []usernautdevv1alpha1.LDAPLeafSubQuery{ + { + Operator: "or", + Filters: []usernautdevv1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrDelta"}, + {Key: "co", Criteria: "equals", Value: "US"}, + }, + }, + }, + }, + }, + }, + } + + got := replaceManagerInSubQueries(queries, "newMgr") + require.Len(t, got, 1) + + assert.Len(t, got[0].Filters, 1) + assert.Equal(t, "newMgr", got[0].Filters[0].Value) + + require.Len(t, got[0].Queries, 1) + assert.Equal(t, "newMgr", got[0].Queries[0].Filters[0].Value) + + require.Len(t, got[0].Queries[0].Queries, 1) + assert.Equal(t, "newMgr", got[0].Queries[0].Queries[0].Filters[0].Value) + assert.Equal(t, "US", got[0].Queries[0].Queries[0].Filters[1].Value) +} + +func TestReplaceManagerInSubQueries_CollapsesDuplicateManagers(t *testing.T) { + t.Parallel() + + query := &usernautdevv1alpha1.LDAPQuery{ + Operator: "and", + Queries: []usernautdevv1alpha1.LDAPSubQuery{ + { + Operator: "or", + Filters: []usernautdevv1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, + {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + }, + }, + }, + } + + got := replaceManagerInSubQueries(query.Queries, "newMgr") + require.Len(t, got, 1) + require.Len(t, got[0].Filters, 1) + assert.Equal(t, "newMgr", got[0].Filters[0].Value) +} + +func TestQueryHasManagerFilter(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + query *usernautdevv1alpha1.LDAPQuery + want bool + }{ + { + name: "nil query", + query: nil, + want: false, + }, + { + name: "top level manager", + query: &usernautdevv1alpha1.LDAPQuery{ + Operator: "and", + Filters: []usernautdevv1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, + }, + }, + want: true, + }, + { + name: "nested manager only", + query: &usernautdevv1alpha1.LDAPQuery{ + Operator: "and", + Queries: []usernautdevv1alpha1.LDAPSubQuery{ + { + Operator: "or", + Filters: []usernautdevv1alpha1.LDAPFilter{ + {Key: "title", Criteria: "contains", Value: "engineer"}, + }, + Queries: []usernautdevv1alpha1.LDAPLeafQuery{ + { + Operator: "and", + Queries: []usernautdevv1alpha1.LDAPLeafSubQuery{ + { + Operator: "or", + Filters: []usernautdevv1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + }, + }, + }, + }, + }, + }, + }, + }, + want: true, + }, + { + name: "no manager anywhere", + query: &usernautdevv1alpha1.LDAPQuery{ + Operator: "and", + Filters: []usernautdevv1alpha1.LDAPFilter{ + {Key: "title", Criteria: "contains", Value: "engineer"}, + }, + }, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, queryHasManagerFilter(tt.query)) + }) + } +} + +func TestExtractManagerUIDsFromQuery(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + query *usernautdevv1alpha1.LDAPQuery + want []string + }{ + { + name: "nil query", + query: nil, + want: nil, + }, + { + name: "top level only", + query: &usernautdevv1alpha1.LDAPQuery{ + Operator: "or", + Filters: []usernautdevv1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, + {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + }, + }, + want: []string{"mgrAlpha", "mgrBeta"}, + }, + { + name: "nested and deduplicated", + query: &usernautdevv1alpha1.LDAPQuery{ + Operator: "and", + Queries: []usernautdevv1alpha1.LDAPSubQuery{ + { + Operator: "or", + Filters: []usernautdevv1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, + }, + Queries: []usernautdevv1alpha1.LDAPLeafQuery{ + { + Operator: "and", + Filters: []usernautdevv1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, + {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + }, + }, + }, + }, + }, + }, + want: []string{"mgrAlpha", "mgrBeta"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, extractManagerUIDsFromQuery(tt.query)) + }) + } +} diff --git a/pkg/clients/ldap/query.go b/pkg/clients/ldap/query.go index d3e3abc9..98f354bc 100644 --- a/pkg/clients/ldap/query.go +++ b/pkg/clients/ldap/query.go @@ -76,6 +76,14 @@ func parseUIDFromDN(dn *ldap.DN) string { return "" } +// QueryNode is an internal uniform tree representation of the typed query hierarchy. +// Exported so the controller can walk it for manager detection and UID extraction. +type QueryNode struct { + Operator string + Filters []v1alpha1.LDAPFilter + Children []*QueryNode +} + func (l *LDAPConn) BuildLDAPQueryFromSpec(ctx context.Context, query *v1alpha1.LDAPQuery) (string, error) { log := logger.Logger(ctx).WithField("build_ldap_query", "spec") log.WithField("query", query).Info("building LDAP query from spec") @@ -83,22 +91,88 @@ func (l *LDAPConn) BuildLDAPQueryFromSpec(ctx context.Context, query *v1alpha1.L if query == nil { return "", errors.New("ldap query is nil") } - if len(query.Filters) == 0 { - return "", errors.New("filters are empty") - } - filters, err := buildFiltersFromSpec(query.Filters, l.baseUserDN) + node, err := ToQueryNode(query) if err != nil { return "", err } + return buildNode(node, l.baseUserDN) +} + +// buildFromSpec is a generic helper that converts any query-like struct into a QueryNode. +func buildFromSpec[T any]( + operator string, + filters []v1alpha1.LDAPFilter, + queries []T, + convertChild func(*T) (*QueryNode, error), +) (*QueryNode, error) { + if len(filters) == 0 && len(queries) == 0 { + return nil, errors.New("filters and queries are both empty") + } + node := &QueryNode{Operator: operator, Filters: filters} + for i := range queries { + child, err := convertChild(&queries[i]) + if err != nil { + return nil, fmt.Errorf("queries[%d]: %w", i, err) + } + node.Children = append(node.Children, child) + } + return node, nil +} + +// ToQueryNode converts the typed LDAPQuery hierarchy into a uniform QueryNode tree. +func ToQueryNode(query *v1alpha1.LDAPQuery) (*QueryNode, error) { + if query == nil { + return nil, errors.New("ldap query is nil") + } + return buildFromSpec(query.Operator, query.Filters, query.Queries, subQueryToNode) +} + +func subQueryToNode(query *v1alpha1.LDAPSubQuery) (*QueryNode, error) { + return buildFromSpec(query.Operator, query.Filters, query.Queries, leafQueryToNode) +} + +func leafQueryToNode(query *v1alpha1.LDAPLeafQuery) (*QueryNode, error) { + return buildFromSpec(query.Operator, query.Filters, query.Queries, leafSubQueryToNode) +} + +func leafSubQueryToNode(query *v1alpha1.LDAPLeafSubQuery) (*QueryNode, error) { + if len(query.Filters) == 0 { + return nil, errors.New("filters are empty") + } + return &QueryNode{Operator: query.Operator, Filters: query.Filters}, nil +} + +func buildNode(node *QueryNode, baseUserDN string) (string, error) { + if len(node.Filters) == 0 && len(node.Children) == 0 { + return "", errors.New("filters and queries are both empty") + } + + parts := make([]string, 0, len(node.Filters)+len(node.Children)) + + if len(node.Filters) > 0 { + filters, err := buildFiltersFromSpec(node.Filters, baseUserDN) + if err != nil { + return "", err + } + parts = append(parts, filters...) + } + + for i, child := range node.Children { + nested, err := buildNode(child, baseUserDN) + if err != nil { + return "", fmt.Errorf("queries[%d]: %w", i, err) + } + parts = append(parts, nested) + } - op := strings.ToLower(strings.TrimSpace(query.Operator)) + op := strings.ToLower(strings.TrimSpace(node.Operator)) switch op { case "and": - return "(&" + strings.Join(filters, "") + ")", nil + return "(&" + strings.Join(parts, "") + ")", nil case "or": - return "(|" + strings.Join(filters, "") + ")", nil + return "(|" + strings.Join(parts, "") + ")", nil default: - return "", fmt.Errorf("unsupported operator %q", query.Operator) + return "", fmt.Errorf("unsupported operator %q", node.Operator) } } diff --git a/pkg/clients/ldap/query_test.go b/pkg/clients/ldap/query_test.go index 0613430d..c30b4700 100644 --- a/pkg/clients/ldap/query_test.go +++ b/pkg/clients/ldap/query_test.go @@ -186,7 +186,7 @@ func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_AndOperator() { { Key: "manager", Criteria: "equals", - Value: "pbhattac", + Value: "mgrAlpha", }, { Key: "title", @@ -199,7 +199,7 @@ func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_AndOperator() { filter, err := ldapConn.BuildLDAPQueryFromSpec(suite.ctx, query) assertions.NoError(err) - assertions.Equal("(&(manager=uid=pbhattac,ou=users,dc=redhat,dc=com)(title=*senior*))", filter) + assertions.Equal("(&(manager=uid=mgrAlpha,ou=users,dc=redhat,dc=com)(title=*senior*))", filter) } func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_OrOperator() { @@ -215,12 +215,12 @@ func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_OrOperator() { { Key: "manager", Criteria: "equals", - Value: "zzhou", + Value: "mgrBeta", }, { Key: "manager", Criteria: "equals", - Value: "robwilli", + Value: "mgrGamma", }, }, } @@ -228,7 +228,7 @@ func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_OrOperator() { assertions.NoError(err) assertions.Equal( - "(|(manager=uid=zzhou,ou=users,dc=redhat,dc=com)(manager=uid=robwilli,ou=users,dc=redhat,dc=com))", + "(|(manager=uid=mgrBeta,ou=users,dc=redhat,dc=com)(manager=uid=mgrGamma,ou=users,dc=redhat,dc=com))", filter, ) } @@ -270,7 +270,7 @@ func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_MixOperator() { { Key: "manager", Criteria: "equals", - Value: "ticramer", + Value: "mgrDelta", }, { Key: "employeeType", @@ -283,5 +283,271 @@ func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_MixOperator() { filter, err := ldapConn.BuildLDAPQueryFromSpec(suite.ctx, query) assertions.NoError(err) - assertions.Equal("(&(manager=uid=ticramer,ou=users,dc=redhat,dc=com)(!(employeeType=external employee)))", filter) + assertions.Equal("(&(manager=uid=mgrDelta,ou=users,dc=redhat,dc=com)(!(employeeType=external employee)))", filter) +} + +func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_NestedOrInsideAnd() { + assertions := assert.New(suite.T()) + + ldapConn := &LDAPConn{ + baseUserDN: "ou=users,dc=redhat,dc=com", + } + + query := &v1alpha1.LDAPQuery{ + Operator: "and", + Filters: []v1alpha1.LDAPFilter{ + { + Key: "employeeType", + Criteria: "not", + Value: "external employee", + }, + }, + Queries: []v1alpha1.LDAPSubQuery{ + { + Operator: "or", + Filters: []v1alpha1.LDAPFilter{ + {Key: "title", Criteria: "contains", Value: "engineer"}, + {Key: "title", Criteria: "contains", Value: "developer"}, + {Key: "title", Criteria: "contains", Value: "architect"}, + }, + }, + }, + } + + filter, err := ldapConn.BuildLDAPQueryFromSpec(suite.ctx, query) + + assertions.NoError(err) + assertions.Equal( + "(&(!(employeeType=external employee))(|(title=*engineer*)(title=*developer*)(title=*architect*)))", + filter, + ) +} + +func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_NestedAndInsideOr() { + assertions := assert.New(suite.T()) + + ldapConn := &LDAPConn{ + baseUserDN: "ou=users,dc=redhat,dc=com", + } + + query := &v1alpha1.LDAPQuery{ + Operator: "or", + Queries: []v1alpha1.LDAPSubQuery{ + { + Operator: "and", + Filters: []v1alpha1.LDAPFilter{ + {Key: "title", Criteria: "contains", Value: "engineer"}, + {Key: "co", Criteria: "equals", Value: "US"}, + }, + }, + { + Operator: "and", + Filters: []v1alpha1.LDAPFilter{ + {Key: "title", Criteria: "contains", Value: "developer"}, + {Key: "co", Criteria: "equals", Value: "IND"}, + }, + }, + }, + } + + filter, err := ldapConn.BuildLDAPQueryFromSpec(suite.ctx, query) + + assertions.NoError(err) + assertions.Equal( + "(|(&(title=*engineer*)(co=US))(&(title=*developer*)(co=IND)))", + filter, + ) +} + +func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_MultipleNestedQueries() { + assertions := assert.New(suite.T()) + + ldapConn := &LDAPConn{ + baseUserDN: "ou=users,dc=redhat,dc=com", + } + + query := &v1alpha1.LDAPQuery{ + Operator: "and", + Filters: []v1alpha1.LDAPFilter{ + {Key: "employeeType", Criteria: "not", Value: "external employee"}, + }, + Queries: []v1alpha1.LDAPSubQuery{ + { + Operator: "or", + Filters: []v1alpha1.LDAPFilter{ + {Key: "title", Criteria: "contains", Value: "engineer"}, + {Key: "title", Criteria: "contains", Value: "developer"}, + }, + }, + { + Operator: "or", + Filters: []v1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, + {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + }, + }, + }, + } + + filter, err := ldapConn.BuildLDAPQueryFromSpec(suite.ctx, query) + + assertions.NoError(err) + expected := "(&(!(employeeType=external employee))" + + "(|(title=*engineer*)(title=*developer*))" + + "(|(manager=uid=mgrAlpha,ou=users,dc=redhat,dc=com)" + + "(manager=uid=mgrBeta,ou=users,dc=redhat,dc=com)))" + assertions.Equal(expected, filter) +} + +func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_QueriesOnly() { + assertions := assert.New(suite.T()) + + ldapConn := &LDAPConn{ + baseUserDN: "ou=users,dc=redhat,dc=com", + } + + query := &v1alpha1.LDAPQuery{ + Operator: "or", + Queries: []v1alpha1.LDAPSubQuery{ + { + Operator: "and", + Filters: []v1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, + }, + }, + { + Operator: "and", + Filters: []v1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + }, + }, + }, + } + + filter, err := ldapConn.BuildLDAPQueryFromSpec(suite.ctx, query) + + assertions.NoError(err) + assertions.Equal( + "(|(&(manager=uid=mgrAlpha,ou=users,dc=redhat,dc=com))(&(manager=uid=mgrBeta,ou=users,dc=redhat,dc=com)))", + filter, + ) +} + +func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_EmptyFiltersAndQueries() { + assertions := assert.New(suite.T()) + + ldapConn := &LDAPConn{ + baseUserDN: "ou=users,dc=redhat,dc=com", + } + + query := &v1alpha1.LDAPQuery{ + Operator: "and", + } + + _, err := ldapConn.BuildLDAPQueryFromSpec(suite.ctx, query) + assertions.Error(err) + assertions.Contains(err.Error(), "filters and queries are both empty") +} + +func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_InvalidNestedOperator() { + assertions := assert.New(suite.T()) + + ldapConn := &LDAPConn{ + baseUserDN: "ou=users,dc=redhat,dc=com", + } + + query := &v1alpha1.LDAPQuery{ + Operator: "and", + Queries: []v1alpha1.LDAPSubQuery{ + { + Operator: "xor", + Filters: []v1alpha1.LDAPFilter{ + {Key: "title", Criteria: "contains", Value: "engineer"}, + }, + }, + }, + } + + _, err := ldapConn.BuildLDAPQueryFromSpec(suite.ctx, query) + assertions.Error(err) + assertions.Contains(err.Error(), "unsupported operator") +} + +func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_EmptyLeafSubQueryFilters() { + assertions := assert.New(suite.T()) + + ldapConn := &LDAPConn{ + baseUserDN: "ou=users,dc=redhat,dc=com", + } + + query := &v1alpha1.LDAPQuery{ + Operator: "and", + Queries: []v1alpha1.LDAPSubQuery{ + { + Operator: "or", + Queries: []v1alpha1.LDAPLeafQuery{ + { + Operator: "and", + Queries: []v1alpha1.LDAPLeafSubQuery{ + { + Operator: "or", + Filters: []v1alpha1.LDAPFilter{}, + }, + }, + }, + }, + }, + }, + } + + _, err := ldapConn.BuildLDAPQueryFromSpec(context.Background(), query) + assertions.Error(err) + assertions.Contains(err.Error(), "filters are empty") +} + +func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_FourLevelNesting() { + assertions := assert.New(suite.T()) + + ldapConn := &LDAPConn{ + baseUserDN: "ou=users,dc=redhat,dc=com", + } + + query := &v1alpha1.LDAPQuery{ + Operator: "and", + Filters: []v1alpha1.LDAPFilter{ + {Key: "employeeType", Criteria: "not", Value: "external employee"}, + }, + Queries: []v1alpha1.LDAPSubQuery{ + { + Operator: "or", + Filters: []v1alpha1.LDAPFilter{ + {Key: "title", Criteria: "contains", Value: "engineer"}, + }, + Queries: []v1alpha1.LDAPLeafQuery{ + { + Operator: "and", + Filters: []v1alpha1.LDAPFilter{ + {Key: "co", Criteria: "equals", Value: "US"}, + }, + Queries: []v1alpha1.LDAPLeafSubQuery{ + { + Operator: "or", + Filters: []v1alpha1.LDAPFilter{ + {Key: "rhatCostCenter", Criteria: "equals", Value: "123"}, + {Key: "rhatCostCenter", Criteria: "equals", Value: "456"}, + }, + }, + }, + }, + }, + }, + }, + } + + filter, err := ldapConn.BuildLDAPQueryFromSpec(suite.ctx, query) + assertions.NoError(err) + assertions.Equal( + "(&(!(employeeType=external employee))(|(title=*engineer*)(&(co=US)(|(rhatCostCenter=123)(rhatCostCenter=456)))))", + filter, + ) } From 3fe8d05a96e19d14db7b8675577ace7389dbe560 Mon Sep 17 00:00:00 2001 From: Abhidas747 Date: Wed, 17 Jun 2026 20:07:58 +0530 Subject: [PATCH 2/8] Updated validation rules in LDAP types to require that at least one of filters or queries is specified and non-empty. Signed-off-by: Abhidas747 --- api/v1alpha1/group_types.go | 11 ++++++----- .../operator.dataverse.redhat.com_groups.yaml | 15 +++++++++++---- internal/controller/group_controller.go | 3 +++ 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/api/v1alpha1/group_types.go b/api/v1alpha1/group_types.go index 15ff092b..263ea9c3 100644 --- a/api/v1alpha1/group_types.go +++ b/api/v1alpha1/group_types.go @@ -44,7 +44,7 @@ type LDAPFilter struct { Value string `json:"value"` } -// +kubebuilder:validation:XValidation:rule="has(self.filters) || has(self.queries)",message="at least one of filters or queries must be specified" +// +kubebuilder:validation:XValidation:rule="(has(self.filters) && size(self.filters) > 0) || (has(self.queries) && size(self.queries) > 0)",message="at least one of filters or queries must be specified and non-empty" type LDAPQuery struct { // +kubebuilder:validation:Enum=and;or Operator string `json:"operator"` @@ -56,7 +56,7 @@ type LDAPQuery struct { Options *LDAPOptions `json:"options,omitempty"` } -// +kubebuilder:validation:XValidation:rule="has(self.filters) || has(self.queries)",message="at least one of filters or queries must be specified" +// +kubebuilder:validation:XValidation:rule="(has(self.filters) && size(self.filters) > 0) || (has(self.queries) && size(self.queries) > 0)",message="at least one of filters or queries must be specified and non-empty" type LDAPSubQuery struct { // +kubebuilder:validation:Enum=and;or Operator string `json:"operator"` @@ -66,7 +66,7 @@ type LDAPSubQuery struct { Queries []LDAPLeafQuery `json:"queries,omitempty"` } -// +kubebuilder:validation:XValidation:rule="has(self.filters) || has(self.queries)",message="at least one of filters or queries must be specified" +// +kubebuilder:validation:XValidation:rule="(has(self.filters) && size(self.filters) > 0) || (has(self.queries) && size(self.queries) > 0)",message="at least one of filters or queries must be specified and non-empty" type LDAPLeafQuery struct { // +kubebuilder:validation:Enum=and;or Operator string `json:"operator"` @@ -78,8 +78,9 @@ type LDAPLeafQuery struct { type LDAPLeafSubQuery struct { // +kubebuilder:validation:Enum=and;or - Operator string `json:"operator"` - Filters []LDAPFilter `json:"filters"` + Operator string `json:"operator"` + // +kubebuilder:validation:MinItems=1 + Filters []LDAPFilter `json:"filters"` } type LDAPOptions struct { diff --git a/config/crd/bases/operator.dataverse.redhat.com_groups.yaml b/config/crd/bases/operator.dataverse.redhat.com_groups.yaml index 7528246d..e64e1d02 100644 --- a/config/crd/bases/operator.dataverse.redhat.com_groups.yaml +++ b/config/crd/bases/operator.dataverse.redhat.com_groups.yaml @@ -262,6 +262,7 @@ spec: - key - value type: object + minItems: 1 type: array operator: enum: @@ -278,22 +279,28 @@ spec: type: object x-kubernetes-validations: - message: at least one of filters or queries must - be specified - rule: has(self.filters) || has(self.queries) + be specified and non-empty + rule: (has(self.filters) && size(self.filters) > + 0) || (has(self.queries) && size(self.queries) + > 0) type: array required: - operator type: object x-kubernetes-validations: - message: at least one of filters or queries must be specified - rule: has(self.filters) || has(self.queries) + and non-empty + rule: (has(self.filters) && size(self.filters) > 0) || + (has(self.queries) && size(self.queries) > 0) type: array required: - operator type: object x-kubernetes-validations: - message: at least one of filters or queries must be specified - rule: has(self.filters) || has(self.queries) + and non-empty + rule: (has(self.filters) && size(self.filters) > 0) || (has(self.queries) + && size(self.queries) > 0) users: items: type: string diff --git a/internal/controller/group_controller.go b/internal/controller/group_controller.go index dcdcb09a..49f13024 100644 --- a/internal/controller/group_controller.go +++ b/internal/controller/group_controller.go @@ -320,6 +320,9 @@ func (r *GroupReconciler) fetchQueryMembers(ctx context.Context, query *usernaut // is replaced with managerUID. Duplicate manager filters with the same criteria and // value after replacement are collapsed to a single entry. func replaceManagerInFilters(filters []usernautdevv1alpha1.LDAPFilter, managerUID string) []usernautdevv1alpha1.LDAPFilter { + if len(filters) == 0 { + return nil + } result := make([]usernautdevv1alpha1.LDAPFilter, 0, len(filters)) seenManagers := make(map[string]struct{}) for _, filter := range filters { From 5d2d70e26138a8fff84658218854f6e4fa37cd5a Mon Sep 17 00:00:00 2001 From: Abhidas747 Date: Thu, 18 Jun 2026 14:31:11 +0530 Subject: [PATCH 3/8] [DATA-6657] Return empty slices instead of nil per repo style guide Signed-off-by: Abhidas747 --- internal/controller/group_controller.go | 12 ++++++------ .../group_controller_manager_query_test.go | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/controller/group_controller.go b/internal/controller/group_controller.go index 49f13024..b4a44746 100644 --- a/internal/controller/group_controller.go +++ b/internal/controller/group_controller.go @@ -321,7 +321,7 @@ func (r *GroupReconciler) fetchQueryMembers(ctx context.Context, query *usernaut // value after replacement are collapsed to a single entry. func replaceManagerInFilters(filters []usernautdevv1alpha1.LDAPFilter, managerUID string) []usernautdevv1alpha1.LDAPFilter { if len(filters) == 0 { - return nil + return []usernautdevv1alpha1.LDAPFilter{} } result := make([]usernautdevv1alpha1.LDAPFilter, 0, len(filters)) seenManagers := make(map[string]struct{}) @@ -347,7 +347,7 @@ func replaceManagerInFilters(filters []usernautdevv1alpha1.LDAPFilter, managerUI // replaceManagerInSubQueries rewrites manager filter values at all nested levels. func replaceManagerInSubQueries(queries []usernautdevv1alpha1.LDAPSubQuery, managerUID string) []usernautdevv1alpha1.LDAPSubQuery { if len(queries) == 0 { - return nil + return []usernautdevv1alpha1.LDAPSubQuery{} } result := make([]usernautdevv1alpha1.LDAPSubQuery, 0, len(queries)) for _, q := range queries { @@ -362,7 +362,7 @@ func replaceManagerInSubQueries(queries []usernautdevv1alpha1.LDAPSubQuery, mana func replaceManagerInLeafQueries(queries []usernautdevv1alpha1.LDAPLeafQuery, managerUID string) []usernautdevv1alpha1.LDAPLeafQuery { if len(queries) == 0 { - return nil + return []usernautdevv1alpha1.LDAPLeafQuery{} } result := make([]usernautdevv1alpha1.LDAPLeafQuery, 0, len(queries)) for _, q := range queries { @@ -377,7 +377,7 @@ func replaceManagerInLeafQueries(queries []usernautdevv1alpha1.LDAPLeafQuery, ma func replaceManagerInLeafSubQueries(queries []usernautdevv1alpha1.LDAPLeafSubQuery, managerUID string) []usernautdevv1alpha1.LDAPLeafSubQuery { if len(queries) == 0 { - return nil + return []usernautdevv1alpha1.LDAPLeafSubQuery{} } result := make([]usernautdevv1alpha1.LDAPLeafSubQuery, 0, len(queries)) for _, q := range queries { @@ -441,10 +441,10 @@ func leafQueryHasManager(query *usernautdevv1alpha1.LDAPLeafQuery) bool { // extractManagerUIDsFromQuery returns unique manager UIDs referenced anywhere in the query tree. func extractManagerUIDsFromQuery(query *usernautdevv1alpha1.LDAPQuery) []string { if query == nil { - return nil + return []string{} } seen := make(map[string]struct{}) - var result []string + result := []string{} collectManagerUIDsFromFilters(query.Filters, seen, &result) for i := range query.Queries { collectSubQueryManagerUIDs(&query.Queries[i], seen, &result) diff --git a/internal/controller/group_controller_manager_query_test.go b/internal/controller/group_controller_manager_query_test.go index 2b75973f..18b899ae 100644 --- a/internal/controller/group_controller_manager_query_test.go +++ b/internal/controller/group_controller_manager_query_test.go @@ -206,7 +206,7 @@ func TestExtractManagerUIDsFromQuery(t *testing.T) { { name: "nil query", query: nil, - want: nil, + want: []string{}, }, { name: "top level only", From 571fb4496710d07cb21632749821c5592496b686 Mon Sep 17 00:00:00 2001 From: Abhidas747 Date: Fri, 19 Jun 2026 12:52:15 +0530 Subject: [PATCH 4/8] [DATA-6657] Rename query fields to level-specific names. Signed-off-by: Abhidas747 --- api/v1alpha1/group_types.go | 12 ++--- .../operator.dataverse.redhat.com_groups.yaml | 45 ++++++++++--------- .../v1alpha1_group_querybased_nested.yaml | 19 +++++++- 3 files changed, 46 insertions(+), 30 deletions(-) diff --git a/api/v1alpha1/group_types.go b/api/v1alpha1/group_types.go index 263ea9c3..9432dff4 100644 --- a/api/v1alpha1/group_types.go +++ b/api/v1alpha1/group_types.go @@ -44,36 +44,36 @@ type LDAPFilter struct { Value string `json:"value"` } -// +kubebuilder:validation:XValidation:rule="(has(self.filters) && size(self.filters) > 0) || (has(self.queries) && size(self.queries) > 0)",message="at least one of filters or queries must be specified and non-empty" +// +kubebuilder:validation:XValidation:rule="(has(self.filters) && size(self.filters) > 0) || (has(self.sub_queries) && size(self.sub_queries) > 0)",message="at least one of filters or sub_queries must be specified and non-empty" type LDAPQuery struct { // +kubebuilder:validation:Enum=and;or Operator string `json:"operator"` // +optional Filters []LDAPFilter `json:"filters,omitempty"` // +optional - Queries []LDAPSubQuery `json:"queries,omitempty"` + Queries []LDAPSubQuery `json:"sub_queries,omitempty"` // +optional Options *LDAPOptions `json:"options,omitempty"` } -// +kubebuilder:validation:XValidation:rule="(has(self.filters) && size(self.filters) > 0) || (has(self.queries) && size(self.queries) > 0)",message="at least one of filters or queries must be specified and non-empty" +// +kubebuilder:validation:XValidation:rule="(has(self.filters) && size(self.filters) > 0) || (has(self.leaf_queries) && size(self.leaf_queries) > 0)",message="at least one of filters or leaf_queries must be specified and non-empty" type LDAPSubQuery struct { // +kubebuilder:validation:Enum=and;or Operator string `json:"operator"` // +optional Filters []LDAPFilter `json:"filters,omitempty"` // +optional - Queries []LDAPLeafQuery `json:"queries,omitempty"` + Queries []LDAPLeafQuery `json:"leaf_queries,omitempty"` } -// +kubebuilder:validation:XValidation:rule="(has(self.filters) && size(self.filters) > 0) || (has(self.queries) && size(self.queries) > 0)",message="at least one of filters or queries must be specified and non-empty" +// +kubebuilder:validation:XValidation:rule="(has(self.filters) && size(self.filters) > 0) || (has(self.leaf_sub_queries) && size(self.leaf_sub_queries) > 0)",message="at least one of filters or leaf_sub_queries must be specified and non-empty" type LDAPLeafQuery struct { // +kubebuilder:validation:Enum=and;or Operator string `json:"operator"` // +optional Filters []LDAPFilter `json:"filters,omitempty"` // +optional - Queries []LDAPLeafSubQuery `json:"queries,omitempty"` + Queries []LDAPLeafSubQuery `json:"leaf_sub_queries,omitempty"` } type LDAPLeafSubQuery struct { diff --git a/config/crd/bases/operator.dataverse.redhat.com_groups.yaml b/config/crd/bases/operator.dataverse.redhat.com_groups.yaml index e64e1d02..103d2e5c 100644 --- a/config/crd/bases/operator.dataverse.redhat.com_groups.yaml +++ b/config/crd/bases/operator.dataverse.redhat.com_groups.yaml @@ -139,7 +139,7 @@ spec: include_manager: type: boolean type: object - queries: + sub_queries: items: properties: filters: @@ -177,12 +177,7 @@ spec: - value type: object type: array - operator: - enum: - - and - - or - type: string - queries: + leaf_queries: items: properties: filters: @@ -220,12 +215,7 @@ spec: - value type: object type: array - operator: - enum: - - and - - or - type: string - queries: + leaf_sub_queries: items: properties: filters: @@ -274,33 +264,44 @@ spec: - operator type: object type: array + operator: + enum: + - and + - or + type: string required: - operator type: object x-kubernetes-validations: - - message: at least one of filters or queries must - be specified and non-empty + - message: at least one of filters or leaf_sub_queries + must be specified and non-empty rule: (has(self.filters) && size(self.filters) > - 0) || (has(self.queries) && size(self.queries) + 0) || (has(self.leaf_sub_queries) && size(self.leaf_sub_queries) > 0) type: array + operator: + enum: + - and + - or + type: string required: - operator type: object x-kubernetes-validations: - - message: at least one of filters or queries must be specified - and non-empty + - message: at least one of filters or leaf_queries must + be specified and non-empty rule: (has(self.filters) && size(self.filters) > 0) || - (has(self.queries) && size(self.queries) > 0) + (has(self.leaf_queries) && size(self.leaf_queries) > + 0) type: array required: - operator type: object x-kubernetes-validations: - - message: at least one of filters or queries must be specified + - message: at least one of filters or sub_queries must be specified and non-empty - rule: (has(self.filters) && size(self.filters) > 0) || (has(self.queries) - && size(self.queries) > 0) + rule: (has(self.filters) && size(self.filters) > 0) || (has(self.sub_queries) + && size(self.sub_queries) > 0) users: items: type: string diff --git a/config/samples/v1alpha1_group_querybased_nested.yaml b/config/samples/v1alpha1_group_querybased_nested.yaml index 4a1775e0..f9ce92c4 100644 --- a/config/samples/v1alpha1_group_querybased_nested.yaml +++ b/config/samples/v1alpha1_group_querybased_nested.yaml @@ -13,12 +13,12 @@ spec: options: include_indirect_reports: true include_manager: true - operator: and + operator: and # Level 0 filters: - key: employeeType criteria: not value: "external employee" - queries: + sub_queries: # Level 1 - operator: or filters: - key: title @@ -35,4 +35,19 @@ spec: - key: manager criteria: equals value: mgrBeta + leaf_queries: # Level 2 + - operator: and + filters: + - key: co + criteria: equals + value: US + leaf_sub_queries: # Level 3 + - operator: or + filters: + - key: rhatGeo + criteria: equals + value: NA + - key: rhatGeo + criteria: equals + value: LATAM backends: [] From d5bbbc6617c8d0888a6c3c5c1c9a5a887d5f3309 Mon Sep 17 00:00:00 2001 From: Abhidas747 Date: Wed, 24 Jun 2026 18:21:04 +0530 Subject: [PATCH 5/8] Embed nested LDAP queries inside filters array Signed-off-by: Abhidas747 --- api/v1alpha1/const.go | 3 + api/v1alpha1/group_types.go | 45 ++--- api/v1alpha1/zz_generated.deepcopy.go | 84 +-------- .../operator.dataverse.redhat.com_groups.yaml | 168 +---------------- .../v1alpha1_group_querybased_nested.yaml | 63 +++---- internal/controller/group_controller.go | 124 +++---------- .../group_controller_manager_query_test.go | 118 ++++++------ pkg/clients/ldap/query.go | 101 ++-------- pkg/clients/ldap/query_test.go | 175 +++++++++++------- 9 files changed, 270 insertions(+), 611 deletions(-) diff --git a/api/v1alpha1/const.go b/api/v1alpha1/const.go index af7fafbb..fd85a7ec 100644 --- a/api/v1alpha1/const.go +++ b/api/v1alpha1/const.go @@ -3,4 +3,7 @@ package v1alpha1 const ( SuccessfullyReconciled = "SuccessfullyReconciled" ReconcileFailed = "ReconcileFailed" + + // MaxLDAPQueryDepth is the maximum nesting depth allowed for ldap_query filters. + MaxLDAPQueryDepth = 4 ) diff --git a/api/v1alpha1/group_types.go b/api/v1alpha1/group_types.go index 9432dff4..2b2d5e21 100644 --- a/api/v1alpha1/group_types.go +++ b/api/v1alpha1/group_types.go @@ -37,50 +37,27 @@ type Backend struct { } type LDAPFilter struct { - // +kubebuilder:validation:Enum=givenName;displayName;rhatJobTitle;title;employeeType;manager;rhatCostCenter;rhatCostCenterDesc;rhatGeo;co;st;rhatLocation;rhatOfficeLocation;rhatOfficeFloor;roomNumber - Key string `json:"key"` - // +kubebuilder:validation:Enum=equals;contains;not - Criteria string `json:"criteria"` - Value string `json:"value"` -} - -// +kubebuilder:validation:XValidation:rule="(has(self.filters) && size(self.filters) > 0) || (has(self.sub_queries) && size(self.sub_queries) > 0)",message="at least one of filters or sub_queries must be specified and non-empty" -type LDAPQuery struct { - // +kubebuilder:validation:Enum=and;or - Operator string `json:"operator"` // +optional - Filters []LDAPFilter `json:"filters,omitempty"` - // +optional - Queries []LDAPSubQuery `json:"sub_queries,omitempty"` - // +optional - Options *LDAPOptions `json:"options,omitempty"` -} - -// +kubebuilder:validation:XValidation:rule="(has(self.filters) && size(self.filters) > 0) || (has(self.leaf_queries) && size(self.leaf_queries) > 0)",message="at least one of filters or leaf_queries must be specified and non-empty" -type LDAPSubQuery struct { - // +kubebuilder:validation:Enum=and;or - Operator string `json:"operator"` - // +optional - Filters []LDAPFilter `json:"filters,omitempty"` + // +kubebuilder:validation:Enum=givenName;displayName;rhatJobTitle;title;employeeType;manager;rhatCostCenter;rhatCostCenterDesc;rhatGeo;co;st;rhatLocation;rhatOfficeLocation;rhatOfficeFloor;roomNumber + Key string `json:"key,omitempty"` // +optional - Queries []LDAPLeafQuery `json:"leaf_queries,omitempty"` -} - -// +kubebuilder:validation:XValidation:rule="(has(self.filters) && size(self.filters) > 0) || (has(self.leaf_sub_queries) && size(self.leaf_sub_queries) > 0)",message="at least one of filters or leaf_sub_queries must be specified and non-empty" -type LDAPLeafQuery struct { - // +kubebuilder:validation:Enum=and;or - Operator string `json:"operator"` + // +kubebuilder:validation:Enum=equals;contains;not + Criteria string `json:"criteria,omitempty"` // +optional - Filters []LDAPFilter `json:"filters,omitempty"` + Value string `json:"value,omitempty"` // +optional - Queries []LDAPLeafSubQuery `json:"leaf_sub_queries,omitempty"` + // +kubebuilder:validation:Schemaless + // +kubebuilder:pruning:PreserveUnknownFields + LDAPQuery *LDAPQuery `json:"ldap_query,omitempty"` } -type LDAPLeafSubQuery struct { +type LDAPQuery struct { // +kubebuilder:validation:Enum=and;or Operator string `json:"operator"` // +kubebuilder:validation:MinItems=1 Filters []LDAPFilter `json:"filters"` + // +optional + Options *LDAPOptions `json:"options,omitempty"` } type LDAPOptions struct { diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 5e003bfc..a2396663 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -197,6 +197,11 @@ func (in *GroupStatus) DeepCopy() *GroupStatus { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *LDAPFilter) DeepCopyInto(out *LDAPFilter) { *out = *in + if in.LDAPQuery != nil { + in, out := &in.LDAPQuery, &out.LDAPQuery + *out = new(LDAPQuery) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LDAPFilter. @@ -209,53 +214,6 @@ func (in *LDAPFilter) DeepCopy() *LDAPFilter { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *LDAPLeafQuery) DeepCopyInto(out *LDAPLeafQuery) { - *out = *in - if in.Filters != nil { - in, out := &in.Filters, &out.Filters - *out = make([]LDAPFilter, len(*in)) - copy(*out, *in) - } - if in.Queries != nil { - in, out := &in.Queries, &out.Queries - *out = make([]LDAPLeafSubQuery, len(*in)) - for i := range *in { - (*in)[i].DeepCopyInto(&(*out)[i]) - } - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LDAPLeafQuery. -func (in *LDAPLeafQuery) DeepCopy() *LDAPLeafQuery { - if in == nil { - return nil - } - out := new(LDAPLeafQuery) - in.DeepCopyInto(out) - return out -} - -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *LDAPLeafSubQuery) DeepCopyInto(out *LDAPLeafSubQuery) { - *out = *in - if in.Filters != nil { - in, out := &in.Filters, &out.Filters - *out = make([]LDAPFilter, len(*in)) - copy(*out, *in) - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LDAPLeafSubQuery. -func (in *LDAPLeafSubQuery) DeepCopy() *LDAPLeafSubQuery { - if in == nil { - return nil - } - out := new(LDAPLeafSubQuery) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *LDAPOptions) DeepCopyInto(out *LDAPOptions) { *out = *in @@ -277,11 +235,6 @@ func (in *LDAPQuery) DeepCopyInto(out *LDAPQuery) { if in.Filters != nil { in, out := &in.Filters, &out.Filters *out = make([]LDAPFilter, len(*in)) - copy(*out, *in) - } - if in.Queries != nil { - in, out := &in.Queries, &out.Queries - *out = make([]LDAPSubQuery, len(*in)) for i := range *in { (*in)[i].DeepCopyInto(&(*out)[i]) } @@ -303,33 +256,6 @@ func (in *LDAPQuery) DeepCopy() *LDAPQuery { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *LDAPSubQuery) DeepCopyInto(out *LDAPSubQuery) { - *out = *in - if in.Filters != nil { - in, out := &in.Filters, &out.Filters - *out = make([]LDAPFilter, len(*in)) - copy(*out, *in) - } - if in.Queries != nil { - in, out := &in.Queries, &out.Queries - *out = make([]LDAPLeafQuery, len(*in)) - for i := range *in { - (*in)[i].DeepCopyInto(&(*out)[i]) - } - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LDAPSubQuery. -func (in *LDAPSubQuery) DeepCopy() *LDAPSubQuery { - if in == nil { - return nil - } - out := new(LDAPSubQuery) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Members) DeepCopyInto(out *Members) { *out = *in diff --git a/config/crd/bases/operator.dataverse.redhat.com_groups.yaml b/config/crd/bases/operator.dataverse.redhat.com_groups.yaml index 103d2e5c..57c5162f 100644 --- a/config/crd/bases/operator.dataverse.redhat.com_groups.yaml +++ b/config/crd/bases/operator.dataverse.redhat.com_groups.yaml @@ -119,13 +119,12 @@ spec: - rhatOfficeFloor - roomNumber type: string + ldap_query: + x-kubernetes-preserve-unknown-fields: true value: type: string - required: - - criteria - - key - - value type: object + minItems: 1 type: array operator: enum: @@ -139,169 +138,10 @@ spec: include_manager: type: boolean type: object - sub_queries: - items: - properties: - filters: - items: - properties: - criteria: - enum: - - equals - - contains - - not - type: string - key: - enum: - - givenName - - displayName - - rhatJobTitle - - title - - employeeType - - manager - - rhatCostCenter - - rhatCostCenterDesc - - rhatGeo - - co - - st - - rhatLocation - - rhatOfficeLocation - - rhatOfficeFloor - - roomNumber - type: string - value: - type: string - required: - - criteria - - key - - value - type: object - type: array - leaf_queries: - items: - properties: - filters: - items: - properties: - criteria: - enum: - - equals - - contains - - not - type: string - key: - enum: - - givenName - - displayName - - rhatJobTitle - - title - - employeeType - - manager - - rhatCostCenter - - rhatCostCenterDesc - - rhatGeo - - co - - st - - rhatLocation - - rhatOfficeLocation - - rhatOfficeFloor - - roomNumber - type: string - value: - type: string - required: - - criteria - - key - - value - type: object - type: array - leaf_sub_queries: - items: - properties: - filters: - items: - properties: - criteria: - enum: - - equals - - contains - - not - type: string - key: - enum: - - givenName - - displayName - - rhatJobTitle - - title - - employeeType - - manager - - rhatCostCenter - - rhatCostCenterDesc - - rhatGeo - - co - - st - - rhatLocation - - rhatOfficeLocation - - rhatOfficeFloor - - roomNumber - type: string - value: - type: string - required: - - criteria - - key - - value - type: object - minItems: 1 - type: array - operator: - enum: - - and - - or - type: string - required: - - filters - - operator - type: object - type: array - operator: - enum: - - and - - or - type: string - required: - - operator - type: object - x-kubernetes-validations: - - message: at least one of filters or leaf_sub_queries - must be specified and non-empty - rule: (has(self.filters) && size(self.filters) > - 0) || (has(self.leaf_sub_queries) && size(self.leaf_sub_queries) - > 0) - type: array - operator: - enum: - - and - - or - type: string - required: - - operator - type: object - x-kubernetes-validations: - - message: at least one of filters or leaf_queries must - be specified and non-empty - rule: (has(self.filters) && size(self.filters) > 0) || - (has(self.leaf_queries) && size(self.leaf_queries) > - 0) - type: array required: + - filters - operator type: object - x-kubernetes-validations: - - message: at least one of filters or sub_queries must be specified - and non-empty - rule: (has(self.filters) && size(self.filters) > 0) || (has(self.sub_queries) - && size(self.sub_queries) > 0) users: items: type: string diff --git a/config/samples/v1alpha1_group_querybased_nested.yaml b/config/samples/v1alpha1_group_querybased_nested.yaml index f9ce92c4..9d0c7cee 100644 --- a/config/samples/v1alpha1_group_querybased_nested.yaml +++ b/config/samples/v1alpha1_group_querybased_nested.yaml @@ -13,41 +13,42 @@ spec: options: include_indirect_reports: true include_manager: true - operator: and # Level 0 + operator: and filters: - key: employeeType criteria: not value: "external employee" - sub_queries: # Level 1 - - operator: or - filters: - - key: title - criteria: contains - value: engineer - - key: title - criteria: contains - value: developer - - operator: or - filters: - - key: manager - criteria: equals - value: mgrAlpha - - key: manager - criteria: equals - value: mgrBeta - leaf_queries: # Level 2 - - operator: and - filters: - - key: co - criteria: equals - value: US - leaf_sub_queries: # Level 3 - - operator: or + - ldap_query: + operator: or + filters: + - key: title + criteria: contains + value: engineer + - key: title + criteria: contains + value: developer + - ldap_query: + operator: or + filters: + - key: manager + criteria: equals + value: mgrAlpha + - key: manager + criteria: equals + value: mgrBeta + - ldap_query: + operator: and filters: - - key: rhatGeo + - key: manager criteria: equals - value: NA - - key: rhatGeo - criteria: equals - value: LATAM + value: mgrGamma + - ldap_query: + operator: or + filters: + - key: rhatGeo + criteria: equals + value: na + - key: rhatGeo + criteria: equals + value: apac backends: [] diff --git a/internal/controller/group_controller.go b/internal/controller/group_controller.go index b4a44746..40d02cdb 100644 --- a/internal/controller/group_controller.go +++ b/internal/controller/group_controller.go @@ -273,9 +273,6 @@ func (r *GroupReconciler) fetchQueryMembers(ctx context.Context, query *usernaut if hasManagerFilter && includeIndirectReports { log.Info("has manager filter, fetching indirect reports") - nestedQuery := usernautdevv1alpha1.LDAPQuery{ - Operator: query.Operator, - } queue := make([]string, 0, len(queryMembers)) queue = append(queue, queryMembers...) @@ -298,8 +295,11 @@ func (r *GroupReconciler) fetchQueryMembers(ctx context.Context, query *usernaut } visited[member] = struct{}{} - nestedQuery.Filters = replaceManagerInFilters(query.Filters, member) - nestedQuery.Queries = replaceManagerInSubQueries(query.Queries, member) + nestedQuery := usernautdevv1alpha1.LDAPQuery{ + Operator: query.Operator, + Filters: replaceManagerInFilters(query.Filters, member), + Options: query.Options, + } nestedQueryMembers, err := r.fetchQueryMembers(ctx, &nestedQuery, includeIndirectReports, visited) if err != nil { log.WithError(err).WithField("manager", member).Error("error fetching indirect reports") @@ -326,6 +326,14 @@ func replaceManagerInFilters(filters []usernautdevv1alpha1.LDAPFilter, managerUI result := make([]usernautdevv1alpha1.LDAPFilter, 0, len(filters)) seenManagers := make(map[string]struct{}) for _, filter := range filters { + if filter.LDAPQuery != nil { + nested := replaceManagerInQuery(filter.LDAPQuery, managerUID) + result = append(result, usernautdevv1alpha1.LDAPFilter{ + LDAPQuery: nested, + }) + continue + } + value := filter.Value if strings.EqualFold(strings.TrimSpace(filter.Key), "manager") { value = managerUID @@ -344,49 +352,15 @@ func replaceManagerInFilters(filters []usernautdevv1alpha1.LDAPFilter, managerUI return result } -// replaceManagerInSubQueries rewrites manager filter values at all nested levels. -func replaceManagerInSubQueries(queries []usernautdevv1alpha1.LDAPSubQuery, managerUID string) []usernautdevv1alpha1.LDAPSubQuery { - if len(queries) == 0 { - return []usernautdevv1alpha1.LDAPSubQuery{} - } - result := make([]usernautdevv1alpha1.LDAPSubQuery, 0, len(queries)) - for _, q := range queries { - result = append(result, usernautdevv1alpha1.LDAPSubQuery{ - Operator: q.Operator, - Filters: replaceManagerInFilters(q.Filters, managerUID), - Queries: replaceManagerInLeafQueries(q.Queries, managerUID), - }) - } - return result -} - -func replaceManagerInLeafQueries(queries []usernautdevv1alpha1.LDAPLeafQuery, managerUID string) []usernautdevv1alpha1.LDAPLeafQuery { - if len(queries) == 0 { - return []usernautdevv1alpha1.LDAPLeafQuery{} - } - result := make([]usernautdevv1alpha1.LDAPLeafQuery, 0, len(queries)) - for _, q := range queries { - result = append(result, usernautdevv1alpha1.LDAPLeafQuery{ - Operator: q.Operator, - Filters: replaceManagerInFilters(q.Filters, managerUID), - Queries: replaceManagerInLeafSubQueries(q.Queries, managerUID), - }) - } - return result -} - -func replaceManagerInLeafSubQueries(queries []usernautdevv1alpha1.LDAPLeafSubQuery, managerUID string) []usernautdevv1alpha1.LDAPLeafSubQuery { - if len(queries) == 0 { - return []usernautdevv1alpha1.LDAPLeafSubQuery{} +func replaceManagerInQuery(query *usernautdevv1alpha1.LDAPQuery, managerUID string) *usernautdevv1alpha1.LDAPQuery { + if query == nil { + return nil } - result := make([]usernautdevv1alpha1.LDAPLeafSubQuery, 0, len(queries)) - for _, q := range queries { - result = append(result, usernautdevv1alpha1.LDAPLeafSubQuery{ - Operator: q.Operator, - Filters: replaceManagerInFilters(q.Filters, managerUID), - }) + return &usernautdevv1alpha1.LDAPQuery{ + Operator: query.Operator, + Filters: replaceManagerInFilters(query.Filters, managerUID), + Options: query.Options, } - return result } func filtersHaveManager(filters []usernautdevv1alpha1.LDAPFilter) bool { @@ -394,6 +368,9 @@ func filtersHaveManager(filters []usernautdevv1alpha1.LDAPFilter) bool { if strings.EqualFold(strings.TrimSpace(filter.Key), "manager") { return true } + if filter.LDAPQuery != nil && queryHasManagerFilter(filter.LDAPQuery) { + return true + } } return false } @@ -403,39 +380,7 @@ func queryHasManagerFilter(query *usernautdevv1alpha1.LDAPQuery) bool { if query == nil { return false } - if filtersHaveManager(query.Filters) { - return true - } - for i := range query.Queries { - if subQueryHasManager(&query.Queries[i]) { - return true - } - } - return false -} - -func subQueryHasManager(query *usernautdevv1alpha1.LDAPSubQuery) bool { - if filtersHaveManager(query.Filters) { - return true - } - for i := range query.Queries { - if leafQueryHasManager(&query.Queries[i]) { - return true - } - } - return false -} - -func leafQueryHasManager(query *usernautdevv1alpha1.LDAPLeafQuery) bool { - if filtersHaveManager(query.Filters) { - return true - } - for i := range query.Queries { - if filtersHaveManager(query.Queries[i].Filters) { - return true - } - } - return false + return filtersHaveManager(query.Filters) } // extractManagerUIDsFromQuery returns unique manager UIDs referenced anywhere in the query tree. @@ -446,14 +391,15 @@ func extractManagerUIDsFromQuery(query *usernautdevv1alpha1.LDAPQuery) []string seen := make(map[string]struct{}) result := []string{} collectManagerUIDsFromFilters(query.Filters, seen, &result) - for i := range query.Queries { - collectSubQueryManagerUIDs(&query.Queries[i], seen, &result) - } return result } func collectManagerUIDsFromFilters(filters []usernautdevv1alpha1.LDAPFilter, seen map[string]struct{}, result *[]string) { for _, filter := range filters { + if filter.LDAPQuery != nil { + collectManagerUIDsFromFilters(filter.LDAPQuery.Filters, seen, result) + continue + } if !strings.EqualFold(strings.TrimSpace(filter.Key), "manager") { continue } @@ -469,20 +415,6 @@ func collectManagerUIDsFromFilters(filters []usernautdevv1alpha1.LDAPFilter, see } } -func collectSubQueryManagerUIDs(query *usernautdevv1alpha1.LDAPSubQuery, seen map[string]struct{}, result *[]string) { - collectManagerUIDsFromFilters(query.Filters, seen, result) - for i := range query.Queries { - collectLeafQueryManagerUIDs(&query.Queries[i], seen, result) - } -} - -func collectLeafQueryManagerUIDs(query *usernautdevv1alpha1.LDAPLeafQuery, seen map[string]struct{}, result *[]string) { - collectManagerUIDsFromFilters(query.Filters, seen, result) - for i := range query.Queries { - collectManagerUIDsFromFilters(query.Queries[i].Filters, seen, result) - } -} - // fetchLDAPData fetches LDAP data for all unique members and populates allLdapUserData. // This function does NOT update any cache indexes - it only fetches data. // If the bulk LDAP client returns an error (e.g. server timeout), the entire reconcile diff --git a/internal/controller/group_controller_manager_query_test.go b/internal/controller/group_controller_manager_query_test.go index 18b899ae..ee4d00b5 100644 --- a/internal/controller/group_controller_manager_query_test.go +++ b/internal/controller/group_controller_manager_query_test.go @@ -58,24 +58,26 @@ func TestReplaceManagerInFilters_PreservesDistinctManagerCriteria(t *testing.T) assert.Equal(t, "newMgr", got[1].Value) } -func TestReplaceManagerInSubQueries(t *testing.T) { +func TestReplaceManagerInFilters_NestedQuery(t *testing.T) { t.Parallel() - queries := []usernautdevv1alpha1.LDAPSubQuery{ + filters := []usernautdevv1alpha1.LDAPFilter{ { - Operator: "or", - Filters: []usernautdevv1alpha1.LDAPFilter{ - {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, - {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + LDAPQuery: &usernautdevv1alpha1.LDAPQuery{ + Operator: "or", + Filters: []usernautdevv1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, + {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + }, }, - Queries: []usernautdevv1alpha1.LDAPLeafQuery{ - { - Operator: "and", - Filters: []usernautdevv1alpha1.LDAPFilter{ - {Key: "manager", Criteria: "equals", Value: "mgrGamma"}, - }, - Queries: []usernautdevv1alpha1.LDAPLeafSubQuery{ - { + }, + { + LDAPQuery: &usernautdevv1alpha1.LDAPQuery{ + Operator: "and", + Filters: []usernautdevv1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrGamma"}, + { + LDAPQuery: &usernautdevv1alpha1.LDAPQuery{ Operator: "or", Filters: []usernautdevv1alpha1.LDAPFilter{ {Key: "manager", Criteria: "equals", Value: "mgrDelta"}, @@ -88,27 +90,26 @@ func TestReplaceManagerInSubQueries(t *testing.T) { }, } - got := replaceManagerInSubQueries(queries, "newMgr") - require.Len(t, got, 1) - - assert.Len(t, got[0].Filters, 1) - assert.Equal(t, "newMgr", got[0].Filters[0].Value) + got := replaceManagerInFilters(filters, "newMgr") + require.Len(t, got, 2) - require.Len(t, got[0].Queries, 1) - assert.Equal(t, "newMgr", got[0].Queries[0].Filters[0].Value) + require.NotNil(t, got[0].LDAPQuery) + assert.Len(t, got[0].LDAPQuery.Filters, 1) + assert.Equal(t, "newMgr", got[0].LDAPQuery.Filters[0].Value) - require.Len(t, got[0].Queries[0].Queries, 1) - assert.Equal(t, "newMgr", got[0].Queries[0].Queries[0].Filters[0].Value) - assert.Equal(t, "US", got[0].Queries[0].Queries[0].Filters[1].Value) + require.NotNil(t, got[1].LDAPQuery) + assert.Equal(t, "newMgr", got[1].LDAPQuery.Filters[0].Value) + require.NotNil(t, got[1].LDAPQuery.Filters[1].LDAPQuery) + assert.Equal(t, "newMgr", got[1].LDAPQuery.Filters[1].LDAPQuery.Filters[0].Value) + assert.Equal(t, "US", got[1].LDAPQuery.Filters[1].LDAPQuery.Filters[1].Value) } -func TestReplaceManagerInSubQueries_CollapsesDuplicateManagers(t *testing.T) { +func TestReplaceManagerInFilters_CollapsesDuplicateManagersInNestedQuery(t *testing.T) { t.Parallel() - query := &usernautdevv1alpha1.LDAPQuery{ - Operator: "and", - Queries: []usernautdevv1alpha1.LDAPSubQuery{ - { + filters := []usernautdevv1alpha1.LDAPFilter{ + { + LDAPQuery: &usernautdevv1alpha1.LDAPQuery{ Operator: "or", Filters: []usernautdevv1alpha1.LDAPFilter{ {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, @@ -118,10 +119,11 @@ func TestReplaceManagerInSubQueries_CollapsesDuplicateManagers(t *testing.T) { }, } - got := replaceManagerInSubQueries(query.Queries, "newMgr") + got := replaceManagerInFilters(filters, "newMgr") require.Len(t, got, 1) - require.Len(t, got[0].Filters, 1) - assert.Equal(t, "newMgr", got[0].Filters[0].Value) + require.NotNil(t, got[0].LDAPQuery) + require.Len(t, got[0].LDAPQuery.Filters, 1) + assert.Equal(t, "newMgr", got[0].LDAPQuery.Filters[0].Value) } func TestQueryHasManagerFilter(t *testing.T) { @@ -151,20 +153,24 @@ func TestQueryHasManagerFilter(t *testing.T) { name: "nested manager only", query: &usernautdevv1alpha1.LDAPQuery{ Operator: "and", - Queries: []usernautdevv1alpha1.LDAPSubQuery{ + Filters: []usernautdevv1alpha1.LDAPFilter{ { - Operator: "or", - Filters: []usernautdevv1alpha1.LDAPFilter{ - {Key: "title", Criteria: "contains", Value: "engineer"}, - }, - Queries: []usernautdevv1alpha1.LDAPLeafQuery{ - { - Operator: "and", - Queries: []usernautdevv1alpha1.LDAPLeafSubQuery{ - { - Operator: "or", + LDAPQuery: &usernautdevv1alpha1.LDAPQuery{ + Operator: "or", + Filters: []usernautdevv1alpha1.LDAPFilter{ + {Key: "title", Criteria: "contains", Value: "engineer"}, + { + LDAPQuery: &usernautdevv1alpha1.LDAPQuery{ + Operator: "and", Filters: []usernautdevv1alpha1.LDAPFilter{ - {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + { + LDAPQuery: &usernautdevv1alpha1.LDAPQuery{ + Operator: "or", + Filters: []usernautdevv1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + }, + }, + }, }, }, }, @@ -223,18 +229,20 @@ func TestExtractManagerUIDsFromQuery(t *testing.T) { name: "nested and deduplicated", query: &usernautdevv1alpha1.LDAPQuery{ Operator: "and", - Queries: []usernautdevv1alpha1.LDAPSubQuery{ + Filters: []usernautdevv1alpha1.LDAPFilter{ { - Operator: "or", - Filters: []usernautdevv1alpha1.LDAPFilter{ - {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, - }, - Queries: []usernautdevv1alpha1.LDAPLeafQuery{ - { - Operator: "and", - Filters: []usernautdevv1alpha1.LDAPFilter{ - {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, - {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + LDAPQuery: &usernautdevv1alpha1.LDAPQuery{ + Operator: "or", + Filters: []usernautdevv1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, + { + LDAPQuery: &usernautdevv1alpha1.LDAPQuery{ + Operator: "and", + Filters: []usernautdevv1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, + {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + }, + }, }, }, }, diff --git a/pkg/clients/ldap/query.go b/pkg/clients/ldap/query.go index 98f354bc..9cbed73d 100644 --- a/pkg/clients/ldap/query.go +++ b/pkg/clients/ldap/query.go @@ -76,14 +76,6 @@ func parseUIDFromDN(dn *ldap.DN) string { return "" } -// QueryNode is an internal uniform tree representation of the typed query hierarchy. -// Exported so the controller can walk it for manager detection and UID extraction. -type QueryNode struct { - Operator string - Filters []v1alpha1.LDAPFilter - Children []*QueryNode -} - func (l *LDAPConn) BuildLDAPQueryFromSpec(ctx context.Context, query *v1alpha1.LDAPQuery) (string, error) { log := logger.Logger(ctx).WithField("build_ldap_query", "spec") log.WithField("query", query).Info("building LDAP query from spec") @@ -91,104 +83,45 @@ func (l *LDAPConn) BuildLDAPQueryFromSpec(ctx context.Context, query *v1alpha1.L if query == nil { return "", errors.New("ldap query is nil") } - node, err := ToQueryNode(query) - if err != nil { - return "", err - } - return buildNode(node, l.baseUserDN) + return buildQueryFromSpec(query, l.baseUserDN, 1) } -// buildFromSpec is a generic helper that converts any query-like struct into a QueryNode. -func buildFromSpec[T any]( - operator string, - filters []v1alpha1.LDAPFilter, - queries []T, - convertChild func(*T) (*QueryNode, error), -) (*QueryNode, error) { - if len(filters) == 0 && len(queries) == 0 { - return nil, errors.New("filters and queries are both empty") - } - node := &QueryNode{Operator: operator, Filters: filters} - for i := range queries { - child, err := convertChild(&queries[i]) - if err != nil { - return nil, fmt.Errorf("queries[%d]: %w", i, err) - } - node.Children = append(node.Children, child) +func buildQueryFromSpec(query *v1alpha1.LDAPQuery, baseUserDN string, depth int) (string, error) { + if depth > v1alpha1.MaxLDAPQueryDepth { + return "", fmt.Errorf("ldap query nesting exceeds maximum depth of %d", v1alpha1.MaxLDAPQueryDepth) } - return node, nil -} - -// ToQueryNode converts the typed LDAPQuery hierarchy into a uniform QueryNode tree. -func ToQueryNode(query *v1alpha1.LDAPQuery) (*QueryNode, error) { - if query == nil { - return nil, errors.New("ldap query is nil") - } - return buildFromSpec(query.Operator, query.Filters, query.Queries, subQueryToNode) -} - -func subQueryToNode(query *v1alpha1.LDAPSubQuery) (*QueryNode, error) { - return buildFromSpec(query.Operator, query.Filters, query.Queries, leafQueryToNode) -} - -func leafQueryToNode(query *v1alpha1.LDAPLeafQuery) (*QueryNode, error) { - return buildFromSpec(query.Operator, query.Filters, query.Queries, leafSubQueryToNode) -} - -func leafSubQueryToNode(query *v1alpha1.LDAPLeafSubQuery) (*QueryNode, error) { if len(query.Filters) == 0 { - return nil, errors.New("filters are empty") + return "", errors.New("filters are empty") } - return &QueryNode{Operator: query.Operator, Filters: query.Filters}, nil -} - -func buildNode(node *QueryNode, baseUserDN string) (string, error) { - if len(node.Filters) == 0 && len(node.Children) == 0 { - return "", errors.New("filters and queries are both empty") - } - - parts := make([]string, 0, len(node.Filters)+len(node.Children)) - if len(node.Filters) > 0 { - filters, err := buildFiltersFromSpec(node.Filters, baseUserDN) + parts := make([]string, 0, len(query.Filters)) + for i, filter := range query.Filters { + part, err := buildFilterItem(filter, baseUserDN, depth) if err != nil { - return "", err + return "", fmt.Errorf("filters[%d]: %w", i, err) } - parts = append(parts, filters...) + parts = append(parts, part) } - for i, child := range node.Children { - nested, err := buildNode(child, baseUserDN) - if err != nil { - return "", fmt.Errorf("queries[%d]: %w", i, err) - } - parts = append(parts, nested) - } - - op := strings.ToLower(strings.TrimSpace(node.Operator)) + op := strings.ToLower(strings.TrimSpace(query.Operator)) switch op { case "and": return "(&" + strings.Join(parts, "") + ")", nil case "or": return "(|" + strings.Join(parts, "") + ")", nil default: - return "", fmt.Errorf("unsupported operator %q", node.Operator) + return "", fmt.Errorf("unsupported operator %q", query.Operator) } } -func buildFiltersFromSpec(filters []v1alpha1.LDAPFilter, baseUserDN string) ([]string, error) { - results := make([]string, 0, len(filters)) - for _, filter := range filters { - result, err := buildFilterFromSpec(filter, baseUserDN) - if err != nil { - return nil, err - } - results = append(results, result) +func buildFilterItem(filter v1alpha1.LDAPFilter, baseUserDN string, depth int) (string, error) { + if filter.LDAPQuery != nil { + return buildQueryFromSpec(filter.LDAPQuery, baseUserDN, depth+1) } - return results, nil + return buildSimpleFilter(filter, baseUserDN) } -func buildFilterFromSpec(filter v1alpha1.LDAPFilter, baseUserDN string) (string, error) { +func buildSimpleFilter(filter v1alpha1.LDAPFilter, baseUserDN string) (string, error) { op := strings.ToLower(strings.TrimSpace(filter.Criteria)) if baseUserDN == "" { diff --git a/pkg/clients/ldap/query_test.go b/pkg/clients/ldap/query_test.go index c30b4700..14cb3144 100644 --- a/pkg/clients/ldap/query_test.go +++ b/pkg/clients/ldap/query_test.go @@ -301,14 +301,14 @@ func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_NestedOrInsideAnd() { Criteria: "not", Value: "external employee", }, - }, - Queries: []v1alpha1.LDAPSubQuery{ { - Operator: "or", - Filters: []v1alpha1.LDAPFilter{ - {Key: "title", Criteria: "contains", Value: "engineer"}, - {Key: "title", Criteria: "contains", Value: "developer"}, - {Key: "title", Criteria: "contains", Value: "architect"}, + LDAPQuery: &v1alpha1.LDAPQuery{ + Operator: "or", + Filters: []v1alpha1.LDAPFilter{ + {Key: "title", Criteria: "contains", Value: "engineer"}, + {Key: "title", Criteria: "contains", Value: "developer"}, + {Key: "title", Criteria: "contains", Value: "architect"}, + }, }, }, }, @@ -332,19 +332,23 @@ func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_NestedAndInsideOr() { query := &v1alpha1.LDAPQuery{ Operator: "or", - Queries: []v1alpha1.LDAPSubQuery{ + Filters: []v1alpha1.LDAPFilter{ { - Operator: "and", - Filters: []v1alpha1.LDAPFilter{ - {Key: "title", Criteria: "contains", Value: "engineer"}, - {Key: "co", Criteria: "equals", Value: "US"}, + LDAPQuery: &v1alpha1.LDAPQuery{ + Operator: "and", + Filters: []v1alpha1.LDAPFilter{ + {Key: "title", Criteria: "contains", Value: "engineer"}, + {Key: "co", Criteria: "equals", Value: "US"}, + }, }, }, { - Operator: "and", - Filters: []v1alpha1.LDAPFilter{ - {Key: "title", Criteria: "contains", Value: "developer"}, - {Key: "co", Criteria: "equals", Value: "IND"}, + LDAPQuery: &v1alpha1.LDAPQuery{ + Operator: "and", + Filters: []v1alpha1.LDAPFilter{ + {Key: "title", Criteria: "contains", Value: "developer"}, + {Key: "co", Criteria: "equals", Value: "IND"}, + }, }, }, }, @@ -370,20 +374,22 @@ func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_MultipleNestedQueries() { Operator: "and", Filters: []v1alpha1.LDAPFilter{ {Key: "employeeType", Criteria: "not", Value: "external employee"}, - }, - Queries: []v1alpha1.LDAPSubQuery{ { - Operator: "or", - Filters: []v1alpha1.LDAPFilter{ - {Key: "title", Criteria: "contains", Value: "engineer"}, - {Key: "title", Criteria: "contains", Value: "developer"}, + LDAPQuery: &v1alpha1.LDAPQuery{ + Operator: "or", + Filters: []v1alpha1.LDAPFilter{ + {Key: "title", Criteria: "contains", Value: "engineer"}, + {Key: "title", Criteria: "contains", Value: "developer"}, + }, }, }, { - Operator: "or", - Filters: []v1alpha1.LDAPFilter{ - {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, - {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + LDAPQuery: &v1alpha1.LDAPQuery{ + Operator: "or", + Filters: []v1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, + {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + }, }, }, }, @@ -408,17 +414,21 @@ func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_QueriesOnly() { query := &v1alpha1.LDAPQuery{ Operator: "or", - Queries: []v1alpha1.LDAPSubQuery{ + Filters: []v1alpha1.LDAPFilter{ { - Operator: "and", - Filters: []v1alpha1.LDAPFilter{ - {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, + LDAPQuery: &v1alpha1.LDAPQuery{ + Operator: "and", + Filters: []v1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrAlpha"}, + }, }, }, { - Operator: "and", - Filters: []v1alpha1.LDAPFilter{ - {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + LDAPQuery: &v1alpha1.LDAPQuery{ + Operator: "and", + Filters: []v1alpha1.LDAPFilter{ + {Key: "manager", Criteria: "equals", Value: "mgrBeta"}, + }, }, }, }, @@ -446,7 +456,7 @@ func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_EmptyFiltersAndQueries() _, err := ldapConn.BuildLDAPQueryFromSpec(suite.ctx, query) assertions.Error(err) - assertions.Contains(err.Error(), "filters and queries are both empty") + assertions.Contains(err.Error(), "filters are empty") } func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_InvalidNestedOperator() { @@ -458,11 +468,13 @@ func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_InvalidNestedOperator() { query := &v1alpha1.LDAPQuery{ Operator: "and", - Queries: []v1alpha1.LDAPSubQuery{ + Filters: []v1alpha1.LDAPFilter{ { - Operator: "xor", - Filters: []v1alpha1.LDAPFilter{ - {Key: "title", Criteria: "contains", Value: "engineer"}, + LDAPQuery: &v1alpha1.LDAPQuery{ + Operator: "xor", + Filters: []v1alpha1.LDAPFilter{ + {Key: "title", Criteria: "contains", Value: "engineer"}, + }, }, }, }, @@ -473,7 +485,7 @@ func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_InvalidNestedOperator() { assertions.Contains(err.Error(), "unsupported operator") } -func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_EmptyLeafSubQueryFilters() { +func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_EmptyNestedFilters() { assertions := assert.New(suite.T()) ldapConn := &LDAPConn{ @@ -482,19 +494,11 @@ func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_EmptyLeafSubQueryFilters( query := &v1alpha1.LDAPQuery{ Operator: "and", - Queries: []v1alpha1.LDAPSubQuery{ + Filters: []v1alpha1.LDAPFilter{ { - Operator: "or", - Queries: []v1alpha1.LDAPLeafQuery{ - { - Operator: "and", - Queries: []v1alpha1.LDAPLeafSubQuery{ - { - Operator: "or", - Filters: []v1alpha1.LDAPFilter{}, - }, - }, - }, + LDAPQuery: &v1alpha1.LDAPQuery{ + Operator: "or", + Filters: []v1alpha1.LDAPFilter{}, }, }, }, @@ -516,25 +520,25 @@ func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_FourLevelNesting() { Operator: "and", Filters: []v1alpha1.LDAPFilter{ {Key: "employeeType", Criteria: "not", Value: "external employee"}, - }, - Queries: []v1alpha1.LDAPSubQuery{ { - Operator: "or", - Filters: []v1alpha1.LDAPFilter{ - {Key: "title", Criteria: "contains", Value: "engineer"}, - }, - Queries: []v1alpha1.LDAPLeafQuery{ - { - Operator: "and", - Filters: []v1alpha1.LDAPFilter{ - {Key: "co", Criteria: "equals", Value: "US"}, - }, - Queries: []v1alpha1.LDAPLeafSubQuery{ - { - Operator: "or", + LDAPQuery: &v1alpha1.LDAPQuery{ + Operator: "or", + Filters: []v1alpha1.LDAPFilter{ + {Key: "title", Criteria: "contains", Value: "engineer"}, + { + LDAPQuery: &v1alpha1.LDAPQuery{ + Operator: "and", Filters: []v1alpha1.LDAPFilter{ - {Key: "rhatCostCenter", Criteria: "equals", Value: "123"}, - {Key: "rhatCostCenter", Criteria: "equals", Value: "456"}, + {Key: "co", Criteria: "equals", Value: "US"}, + { + LDAPQuery: &v1alpha1.LDAPQuery{ + Operator: "or", + Filters: []v1alpha1.LDAPFilter{ + {Key: "rhatCostCenter", Criteria: "equals", Value: "123"}, + {Key: "rhatCostCenter", Criteria: "equals", Value: "456"}, + }, + }, + }, }, }, }, @@ -551,3 +555,38 @@ func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_FourLevelNesting() { filter, ) } + +func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_ExceedsMaxDepth() { + assertions := assert.New(suite.T()) + + ldapConn := &LDAPConn{ + baseUserDN: "ou=users,dc=redhat,dc=com", + } + + level4 := &v1alpha1.LDAPQuery{ + Operator: "and", + Filters: []v1alpha1.LDAPFilter{ + {Key: "co", Criteria: "equals", Value: "US"}, + }, + } + level3 := &v1alpha1.LDAPQuery{ + Operator: "or", + Filters: []v1alpha1.LDAPFilter{{LDAPQuery: level4}}, + } + level2 := &v1alpha1.LDAPQuery{ + Operator: "and", + Filters: []v1alpha1.LDAPFilter{{LDAPQuery: level3}}, + } + level1 := &v1alpha1.LDAPQuery{ + Operator: "or", + Filters: []v1alpha1.LDAPFilter{{LDAPQuery: level2}}, + } + query := &v1alpha1.LDAPQuery{ + Operator: "and", + Filters: []v1alpha1.LDAPFilter{{LDAPQuery: level1}}, + } + + _, err := ldapConn.BuildLDAPQueryFromSpec(suite.ctx, query) + assertions.Error(err) + assertions.Contains(err.Error(), "exceeds maximum depth") +} From 169dcf3e1e397c98bea1a0ad7cecffe5457094a1 Mon Sep 17 00:00:00 2001 From: Abhidas747 Date: Wed, 8 Jul 2026 21:49:05 +0530 Subject: [PATCH 6/8] Enhance LDAP filter validation to prevent simultaneous use of key/criteria/value and nested LDAP queries. Signed-off-by: Abhidas747 --- pkg/clients/ldap/query.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/clients/ldap/query.go b/pkg/clients/ldap/query.go index 9cbed73d..10055a37 100644 --- a/pkg/clients/ldap/query.go +++ b/pkg/clients/ldap/query.go @@ -115,7 +115,14 @@ func buildQueryFromSpec(query *v1alpha1.LDAPQuery, baseUserDN string, depth int) } func buildFilterItem(filter v1alpha1.LDAPFilter, baseUserDN string, depth int) (string, error) { - if filter.LDAPQuery != nil { + hasSimple := filter.Key != "" || filter.Criteria != "" || filter.Value != "" + hasNested := filter.LDAPQuery != nil + + if hasSimple && hasNested { + return "", errors.New("filter item cannot have both key/criteria/value and ldap_query") + } + + if hasNested { return buildQueryFromSpec(filter.LDAPQuery, baseUserDN, depth+1) } return buildSimpleFilter(filter, baseUserDN) From f9bf67a5cc2df18b5c8c2640ddc3c54f55b47a8e Mon Sep 17 00:00:00 2001 From: Abhidas747 Date: Mon, 13 Jul 2026 17:21:05 +0530 Subject: [PATCH 7/8] Added validation unit test for rejecting filter items with both key/criteria/value and ldap_query Signed-off-by: Abhidas747 --- pkg/clients/ldap/query_test.go | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/pkg/clients/ldap/query_test.go b/pkg/clients/ldap/query_test.go index 14cb3144..6a2e1554 100644 --- a/pkg/clients/ldap/query_test.go +++ b/pkg/clients/ldap/query_test.go @@ -509,6 +509,37 @@ func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_EmptyNestedFilters() { assertions.Contains(err.Error(), "filters are empty") } +// TestBuildLDAPQueryFromSpec_BothSimpleAndNestedOnSameFilter verifies buildFilterItem rejects +// a filter item that sets both key/criteria/value and ldap_query. +func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_BothSimpleAndNestedOnSameFilter() { + assertions := assert.New(suite.T()) + + ldapConn := &LDAPConn{ + baseUserDN: "ou=users,dc=redhat,dc=com", + } + + query := &v1alpha1.LDAPQuery{ + Operator: "and", + Filters: []v1alpha1.LDAPFilter{ + { + Key: "title", + Criteria: "contains", + Value: "engineer", + LDAPQuery: &v1alpha1.LDAPQuery{ + Operator: "or", + Filters: []v1alpha1.LDAPFilter{ + {Key: "rhatGeo", Criteria: "equals", Value: "APAC"}, + }, + }, + }, + }, + } + + _, err := ldapConn.BuildLDAPQueryFromSpec(suite.ctx, query) + assertions.Error(err) + assertions.Contains(err.Error(), "filter item cannot have both key/criteria/value and ldap_query") +} + func (suite *LDAPTestSuite) TestBuildLDAPQueryFromSpec_FourLevelNesting() { assertions := assert.New(suite.T()) From ca398d879e7c7c83c60b3f15f19190129308a57b Mon Sep 17 00:00:00 2001 From: Abhidas747 Date: Wed, 15 Jul 2026 14:42:54 +0530 Subject: [PATCH 8/8] Trigger CI re-run Signed-off-by: Abhidas747