Skip to content

Commit 37c317a

Browse files
JoseSzychoJoseSzycho
authored
feat: Introduce a new filter to exclude private ContactGroups (#494)
Updates the ContactGroup visibility filter to strictly enforce spec.visibility=public for user-scoped requests. - Private contact groups are now hidden from this endpoint regardless of membership. - Updated unit tests and E2E scenarios to verify strict public-only filtering. Relates to: https://datum-inc.slack.com/archives/C084W6XRVS7/p1770121357391059
2 parents 0ba27a6 + 1bd0eff commit 37c317a

10 files changed

Lines changed: 272 additions & 42 deletions

File tree

‎cmd/milo/apiserver/config.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -402,7 +402,7 @@ func DefaultBuildHandlerChain(apiHandler http.Handler, c *server.Config, loopbac
402402
handler = datumfilters.UserContactListConstraintDecorator(handler)
403403
handler = datumfilters.UserContactGroupMembershipListConstraintDecorator(handler)
404404
handler = datumfilters.UserContactGroupMembershipRemovalListConstraintDecorator(handler)
405-
handler = datumfilters.ContactGroupVisibilityDecorator(loopbackConfig)(handler)
405+
handler = datumfilters.ContactGroupVisibilityWithoutPrivateDecorator(handler)
406406
handler = genericapifilters.WithRequestInfo(handler, c.RequestInfoResolver)
407407
handler = genericapifilters.WithRequestReceivedTimestamp(handler)
408408
// handler = genericapifilters.WithMuxAndDiscoveryComplete(handler, c.lifecycleSignals.MuxAndDiscoveryComplete.Signaled())

‎config/crd/bases/notification/notification.miloapis.com_contactgroups.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,8 @@ spec:
216216
type: array
217217
type: object
218218
type: object
219+
selectableFields:
220+
- jsonPath: .spec.visibility
219221
served: true
220222
storage: true
221223
subresources:

‎pkg/apis/notification/v1alpha1/contactgroup_types.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ const (
5252
// +kubebuilder:printcolumn:name="Ready",type="string",JSONPath=".status.conditions[?(@.type=='Ready')].status"
5353
// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"
5454
// +kubebuilder:resource:scope=Namespaced
55+
// +kubebuilder:selectablefield:JSONPath=".spec.visibility"
5556
type ContactGroup struct {
5657
metav1.TypeMeta `json:",inline"`
5758
metav1.ObjectMeta `json:"metadata,omitempty"`

‎pkg/server/filters/contactgroups_excludeprivates.go‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
package filters
2+
3+
import (
4+
"fmt"
5+
"net/http"
6+
"net/url"
7+
8+
"k8s.io/apimachinery/pkg/fields"
9+
"k8s.io/apiserver/pkg/endpoints/handlers/responsewriters"
10+
"k8s.io/apiserver/pkg/endpoints/request"
11+
12+
notificationv1alpha1 "go.miloapis.com/milo/pkg/apis/notification/v1alpha1"
13+
)
14+
15+
const (
16+
// ContactGroupVisibilityFieldSelector is the field selector for the visibility in a contact group.
17+
ContactGroupVisibilityFieldSelector = "spec.visibility"
18+
)
19+
20+
// ContactGroupVisibilityWithoutPrivateDecorator intercepts requests to list
21+
// contact groups, and injects a field selector to only include public contact groups.
22+
//
23+
// This is done so that end users can execute `kubectl get contactgroups`
24+
// and only see public contact groups.
25+
func ContactGroupVisibilityWithoutPrivateDecorator(handler http.Handler) http.Handler {
26+
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
27+
ctx := req.Context()
28+
info, ok := request.RequestInfoFrom(ctx)
29+
if !ok {
30+
responsewriters.InternalError(w, req, fmt.Errorf("failed to get RequestInfo from context"))
31+
return
32+
}
33+
34+
if info.APIGroup == notificationv1alpha1.SchemeGroupVersion.Group && info.Resource == "contactgroups" && info.Verb == "list" {
35+
_, ok := ctx.Value(UserIDContextKey).(string)
36+
if ok {
37+
currentSelector, err := fields.ParseSelector(info.FieldSelector)
38+
if err != nil {
39+
responsewriters.InternalError(w, req, fmt.Errorf("failed to parse label selector: %w", err))
40+
return
41+
}
42+
43+
// Filter out any subject constraints that may have been provided
44+
// in the request by rebuilding the selector without them.
45+
filteredSelector := fields.Nothing()
46+
for _, r := range currentSelector.Requirements() {
47+
if r.Field == ContactGroupVisibilityFieldSelector {
48+
// Skip any pre-existing subject constraints so we can
49+
// replace it with the authenticated user's ID.
50+
continue
51+
}
52+
filteredSelector = fields.AndSelectors(filteredSelector, fields.OneTermEqualSelector(r.Field, r.Value))
53+
}
54+
55+
// Combine the filtered selector with the new subject requirements.
56+
currentSelector = filteredSelector
57+
58+
// Build new selector, filtering out any user-id/kind constraint that
59+
// may have been provided in the request
60+
newSelector := fields.AndSelectors(currentSelector, fields.SelectorFromSet(fields.Set{
61+
ContactGroupVisibilityFieldSelector: "public",
62+
}))
63+
64+
// Set the new field selector on the request info.
65+
info.FieldSelector = newSelector.String()
66+
67+
// Inject the new selector into the request
68+
query, err := url.ParseQuery(req.URL.RawQuery)
69+
if err != nil {
70+
responsewriters.InternalError(w, req, fmt.Errorf("failed to parse url query: %w", err))
71+
return
72+
}
73+
query.Del("fieldSelector")
74+
query.Add("fieldSelector", info.FieldSelector)
75+
76+
req.URL.RawQuery = query.Encode()
77+
}
78+
}
79+
80+
req = req.WithContext(request.WithRequestInfo(ctx, info))
81+
82+
handler.ServeHTTP(w, req)
83+
})
84+
}

‎pkg/server/filters/contactgroups_excludeprivates_test.go‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
package filters
2+
3+
import (
4+
"net/http"
5+
"net/http/httptest"
6+
"net/url"
7+
"testing"
8+
9+
"k8s.io/apiserver/pkg/endpoints/request"
10+
11+
notificationv1alpha1 "go.miloapis.com/milo/pkg/apis/notification/v1alpha1"
12+
)
13+
14+
func TestContactGroupVisibilityWithoutPrivateDecorator(t *testing.T) {
15+
testCases := []struct {
16+
name string
17+
requestPath string
18+
apiGroup string
19+
resource string
20+
verb string
21+
userID string
22+
existingFieldSelector string
23+
expectedFieldSelector string
24+
}{
25+
{
26+
name: "contactgroups list with user context",
27+
apiGroup: notificationv1alpha1.SchemeGroupVersion.Group,
28+
resource: "contactgroups",
29+
verb: "list",
30+
userID: "test-user",
31+
existingFieldSelector: "",
32+
expectedFieldSelector: ",spec.visibility=public",
33+
},
34+
{
35+
name: "contactgroups list with existing field selector",
36+
apiGroup: notificationv1alpha1.SchemeGroupVersion.Group,
37+
resource: "contactgroups",
38+
verb: "list",
39+
userID: "test-user",
40+
existingFieldSelector: "metadata.name=test-group",
41+
expectedFieldSelector: ",metadata.name=test-group,spec.visibility=public",
42+
},
43+
{
44+
name: "existing visibility filter replaced",
45+
requestPath: "/apis/notification.miloapis.com/v1alpha1/contactgroups",
46+
apiGroup: notificationv1alpha1.SchemeGroupVersion.Group,
47+
resource: "contactgroups",
48+
verb: "list",
49+
userID: "test-user",
50+
existingFieldSelector: "spec.visibility=private",
51+
expectedFieldSelector: ",spec.visibility=public",
52+
},
53+
{
54+
name: "non-contactgroups request",
55+
requestPath: "/api/v1/pods",
56+
apiGroup: "",
57+
resource: "pods",
58+
verb: "list",
59+
userID: "test-user",
60+
},
61+
{
62+
name: "contactgroups get request",
63+
requestPath: "/apis/notification.miloapis.com/v1alpha1/contactgroups/test-group",
64+
apiGroup: notificationv1alpha1.SchemeGroupVersion.Group,
65+
resource: "contactgroups",
66+
verb: "get",
67+
userID: "test-user",
68+
},
69+
{
70+
name: "contactgroups list without user context",
71+
apiGroup: notificationv1alpha1.SchemeGroupVersion.Group,
72+
resource: "contactgroups",
73+
verb: "list",
74+
userID: "",
75+
existingFieldSelector: "",
76+
expectedFieldSelector: "", // Should not be modified
77+
},
78+
}
79+
80+
for _, tc := range testCases {
81+
t.Run(tc.name, func(t *testing.T) {
82+
var capturedFieldSelector string
83+
84+
handler := ContactGroupVisibilityWithoutPrivateDecorator(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
85+
if info, ok := request.RequestInfoFrom(req.Context()); ok {
86+
capturedFieldSelector = info.FieldSelector
87+
}
88+
}))
89+
90+
requestURL := tc.requestPath
91+
if tc.existingFieldSelector != "" {
92+
u, _ := url.Parse(requestURL)
93+
query := u.Query()
94+
query.Set("fieldSelector", tc.existingFieldSelector)
95+
u.RawQuery = query.Encode()
96+
requestURL = u.String()
97+
}
98+
99+
req := httptest.NewRequest("GET", "http://localhost"+requestURL, nil)
100+
ctx := req.Context()
101+
102+
requestInfo := &request.RequestInfo{
103+
IsResourceRequest: true,
104+
APIGroup: tc.apiGroup,
105+
Resource: tc.resource,
106+
Verb: tc.verb,
107+
FieldSelector: tc.existingFieldSelector,
108+
}
109+
110+
ctx = request.WithRequestInfo(ctx, requestInfo)
111+
112+
if tc.userID != "" {
113+
ctx = request.WithValue(ctx, UserIDContextKey, tc.userID)
114+
}
115+
116+
req = req.WithContext(ctx)
117+
w := httptest.NewRecorder()
118+
119+
handler.ServeHTTP(w, req)
120+
121+
if tc.expectedFieldSelector != "" {
122+
if capturedFieldSelector != tc.expectedFieldSelector {
123+
t.Fatalf("expected field selector %q, got %q", tc.expectedFieldSelector, capturedFieldSelector)
124+
}
125+
} else if tc.expectedFieldSelector == "" {
126+
if capturedFieldSelector != tc.existingFieldSelector {
127+
t.Fatalf("expected field selector to be unchanged %q, got %q", tc.existingFieldSelector, capturedFieldSelector)
128+
}
129+
}
130+
})
131+
}
132+
}

pkg/server/filters/contactgroups.go renamed to pkg/server/filters/contactgroups_includeprivates.go

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,14 @@ import (
2626
// - Public contact groups are visible to all users
2727
// - Private contact groups are only visible to users who have an associated
2828
// ContactGroupMembership
29-
type ContactGroupVisibilityFilter struct {
29+
type ContactGroupVisibilityWithPrivateFilter struct {
3030
loopbackConfig *rest.Config
3131
clientGetter func() (dynamic.Interface, error)
3232
}
3333

3434
// NewContactGroupVisibilityFilter creates a new ContactGroupVisibilityFilter.
35-
func NewContactGroupVisibilityFilter(loopbackConfig *rest.Config) *ContactGroupVisibilityFilter {
36-
return &ContactGroupVisibilityFilter{
35+
func NewContactGroupVisibilityWithPrivateFilter(loopbackConfig *rest.Config) *ContactGroupVisibilityWithPrivateFilter {
36+
return &ContactGroupVisibilityWithPrivateFilter{
3737
loopbackConfig: loopbackConfig,
3838
clientGetter: func() (dynamic.Interface, error) {
3939
return dynamic.NewForConfig(loopbackConfig)
@@ -47,13 +47,13 @@ func NewContactGroupVisibilityFilter(loopbackConfig *rest.Config) *ContactGroupV
4747
// Visibility rules:
4848
// - Public groups: visible to everyone
4949
// - Private groups: only visible if the user has a ContactGroupMembership associated with that group
50-
func ContactGroupVisibilityDecorator(loopbackConfig *rest.Config) func(http.Handler) http.Handler {
51-
filter := NewContactGroupVisibilityFilter(loopbackConfig)
50+
func ContactGroupVisibilityWithPrivateDecorator(loopbackConfig *rest.Config) func(http.Handler) http.Handler {
51+
filter := NewContactGroupVisibilityWithPrivateFilter(loopbackConfig)
5252
return filter.Wrap
5353
}
5454

5555
// Wrap wraps the provided handler with contact group visibility filtering.
56-
func (f *ContactGroupVisibilityFilter) Wrap(handler http.Handler) http.Handler {
56+
func (f *ContactGroupVisibilityWithPrivateFilter) Wrap(handler http.Handler) http.Handler {
5757
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
5858
ctx := req.Context()
5959
info, ok := request.RequestInfoFrom(ctx)
@@ -104,7 +104,7 @@ func (f *ContactGroupVisibilityFilter) Wrap(handler http.Handler) http.Handler {
104104

105105
// filterContactGroups filters the list of contact groups based on visibility rules.
106106
// Supports both ContactGroupList/List responses and Table responses (kubectl default).
107-
func (f *ContactGroupVisibilityFilter) filterContactGroups(ctx context.Context, userID string, body []byte) ([]byte, error) {
107+
func (f *ContactGroupVisibilityWithPrivateFilter) filterContactGroups(ctx context.Context, userID string, body []byte) ([]byte, error) {
108108
// First, check the response kind to determine how to filter it.
109109
var typeMeta struct {
110110
Kind string `json:"kind"`
@@ -135,7 +135,7 @@ func (f *ContactGroupVisibilityFilter) filterContactGroups(ctx context.Context,
135135

136136
// buildAccessibleGroupsSet returns the set of group keys (namespace/name) accessible to the user.
137137
// Returns the map and a boolean indicating if the user has an associated Contact.
138-
func (f *ContactGroupVisibilityFilter) buildAccessibleGroupsSet(ctx context.Context, client dynamic.Interface, userID string) (map[string]bool, bool) {
138+
func (f *ContactGroupVisibilityWithPrivateFilter) buildAccessibleGroupsSet(ctx context.Context, client dynamic.Interface, userID string) (map[string]bool, bool) {
139139
contactName, contactNamespace, err := f.findContactForUser(ctx, client, userID)
140140
if err != nil {
141141
// No contact found, user can only see public groups
@@ -152,7 +152,7 @@ func (f *ContactGroupVisibilityFilter) buildAccessibleGroupsSet(ctx context.Cont
152152
}
153153

154154
// filterListResponse filters a ContactGroupList or List response.
155-
func (f *ContactGroupVisibilityFilter) filterListResponse(body []byte, accessibleGroups map[string]bool, hasContact bool) ([]byte, error) {
155+
func (f *ContactGroupVisibilityWithPrivateFilter) filterListResponse(body []byte, accessibleGroups map[string]bool, hasContact bool) ([]byte, error) {
156156
var contactGroupList notificationv1alpha1.ContactGroupList
157157
if err := json.Unmarshal(body, &contactGroupList); err != nil {
158158
return nil, fmt.Errorf("failed to unmarshal contact group list: %w", err)
@@ -164,7 +164,7 @@ func (f *ContactGroupVisibilityFilter) filterListResponse(body []byte, accessibl
164164
}
165165

166166
// filterTableResponse filters a Table response (kubectl default format).
167-
func (f *ContactGroupVisibilityFilter) filterTableResponse(ctx context.Context, client dynamic.Interface, body []byte, accessibleGroups map[string]bool, hasContact bool) ([]byte, error) {
167+
func (f *ContactGroupVisibilityWithPrivateFilter) filterTableResponse(ctx context.Context, client dynamic.Interface, body []byte, accessibleGroups map[string]bool, hasContact bool) ([]byte, error) {
168168
var table metav1.Table
169169
if err := json.Unmarshal(body, &table); err != nil {
170170
return nil, fmt.Errorf("failed to unmarshal table: %w", err)
@@ -210,7 +210,7 @@ func (f *ContactGroupVisibilityFilter) filterTableResponse(ctx context.Context,
210210
}
211211

212212
// filterGroupItems filters a slice of ContactGroup items based on visibility rules.
213-
func (f *ContactGroupVisibilityFilter) filterGroupItems(groups []notificationv1alpha1.ContactGroup, accessibleGroups map[string]bool, hasContact bool) []notificationv1alpha1.ContactGroup {
213+
func (f *ContactGroupVisibilityWithPrivateFilter) filterGroupItems(groups []notificationv1alpha1.ContactGroup, accessibleGroups map[string]bool, hasContact bool) []notificationv1alpha1.ContactGroup {
214214
filtered := make([]notificationv1alpha1.ContactGroup, 0, len(groups))
215215
for _, group := range groups {
216216
if f.isGroupVisible(group, accessibleGroups, hasContact) {
@@ -221,7 +221,7 @@ func (f *ContactGroupVisibilityFilter) filterGroupItems(groups []notificationv1a
221221
}
222222

223223
// isGroupVisible determines if a contact group is visible to the user.
224-
func (f *ContactGroupVisibilityFilter) isGroupVisible(group notificationv1alpha1.ContactGroup, accessibleGroups map[string]bool, hasContact bool) bool {
224+
func (f *ContactGroupVisibilityWithPrivateFilter) isGroupVisible(group notificationv1alpha1.ContactGroup, accessibleGroups map[string]bool, hasContact bool) bool {
225225
// Public groups are always visible
226226
if group.Spec.Visibility == notificationv1alpha1.ContactGroupVisibilityPublic {
227227
return true
@@ -238,7 +238,7 @@ func (f *ContactGroupVisibilityFilter) isGroupVisible(group notificationv1alpha1
238238

239239
// findContactForUser finds the Contact resource for the given user ID.
240240
// Searches across all namespaces and returns the contact name, namespace, and any error.
241-
func (f *ContactGroupVisibilityFilter) findContactForUser(ctx context.Context, client dynamic.Interface, userID string) (string, string, error) {
241+
func (f *ContactGroupVisibilityWithPrivateFilter) findContactForUser(ctx context.Context, client dynamic.Interface, userID string) (string, string, error) {
242242
// Query contacts across all namespaces
243243
contactGVR := notificationv1alpha1.SchemeGroupVersion.WithResource("contacts")
244244

@@ -265,7 +265,7 @@ func (f *ContactGroupVisibilityFilter) findContactForUser(ctx context.Context, c
265265

266266
// getAccessibleGroups returns the list of contact group keys (namespace/name) that the user has access to
267267
// (through membership or removal records). Searches across all namespaces.
268-
func (f *ContactGroupVisibilityFilter) getAccessibleGroups(ctx context.Context, client dynamic.Interface, contactName, contactNamespace string) (map[string]bool, error) {
268+
func (f *ContactGroupVisibilityWithPrivateFilter) getAccessibleGroups(ctx context.Context, client dynamic.Interface, contactName, contactNamespace string) (map[string]bool, error) {
269269
accessibleGroups := make(map[string]bool)
270270

271271
// Get memberships for this contact across all namespaces

pkg/server/filters/contactgroups_test.go renamed to pkg/server/filters/contactgroups_includeprivates_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import (
2121
)
2222

2323
func TestFilterGroupItems(t *testing.T) {
24-
f := NewContactGroupVisibilityFilter(nil)
24+
f := NewContactGroupVisibilityWithPrivateFilter(nil)
2525

2626
tests := []struct {
2727
name string
@@ -340,7 +340,7 @@ func TestContactGroupVisibilityFilter_Visibility(t *testing.T) {
340340
for _, tc := range tests {
341341
t.Run(tc.name, func(t *testing.T) {
342342
// Setup fake client
343-
f := NewContactGroupVisibilityFilter(nil)
343+
f := NewContactGroupVisibilityWithPrivateFilter(nil)
344344
f.clientGetter = func() (dynamic.Interface, error) {
345345
// Convert typed objects to unstructured for the dynamic client
346346
scheme := runtime.NewScheme()

0 commit comments

Comments
 (0)