From abddca2c1132eefdf744afadf175d16dd24b2afc Mon Sep 17 00:00:00 2001 From: pasha-codefresh Date: Tue, 18 Jul 2023 17:37:27 +0300 Subject: [PATCH] fix(server): not need send application if it is not under enabled namespaces (#14479) * fix: not need send application if it is not under enabled namespaces * fix condition * feat: Move application is permitted outside of watch function and cover with unit tests * feat: Move application is permitted outside of watch function and cover with unit tests --- server/application/application.go | 44 +++++++++++++-------- server/application/application_test.go | 54 ++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 16 deletions(-) diff --git a/server/application/application.go b/server/application/application.go index 67ceee1716994..8097185a7cb33 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -49,7 +49,6 @@ import ( "github.com/argoproj/argo-cd/v2/util/db" "github.com/argoproj/argo-cd/v2/util/env" "github.com/argoproj/argo-cd/v2/util/git" - "github.com/argoproj/argo-cd/v2/util/glob" ioutil "github.com/argoproj/argo-cd/v2/util/io" "github.com/argoproj/argo-cd/v2/util/lua" "github.com/argoproj/argo-cd/v2/util/manifeststream" @@ -224,7 +223,7 @@ func (s *Server) List(ctx context.Context, q *application.ApplicationQuery) (*ap for _, a := range filteredApps { // Skip any application that is neither in the control plane's namespace // nor in the list of enabled namespaces. - if a.Namespace != s.ns && !glob.MatchStringInList(s.enabledNamespaces, a.Namespace, false) { + if !s.isNamespaceEnabled(a.Namespace) { continue } if s.enf.Enforce(ctx.Value("claims"), rbacpolicy.ResourceApplications, rbacpolicy.ActionGet, a.RBACName(s.ns)) { @@ -980,6 +979,31 @@ func (s *Server) Delete(ctx context.Context, q *application.ApplicationDeleteReq return &application.ApplicationResponse{}, nil } +func (s *Server) isApplicationPermitted(selector labels.Selector, minVersion int, claims any, appName, appNs string, projects map[string]bool, a appv1.Application) bool { + if len(projects) > 0 && !projects[a.Spec.GetProject()] { + return false + } + + if appVersion, err := strconv.Atoi(a.ResourceVersion); err == nil && appVersion < minVersion { + return false + } + matchedEvent := (appName == "" || (a.Name == appName && a.Namespace == appNs)) && selector.Matches(labels.Set(a.Labels)) + if !matchedEvent { + return false + } + + if !s.isNamespaceEnabled(a.Namespace) { + return false + } + + if !s.enf.Enforce(claims, rbacpolicy.ResourceApplications, rbacpolicy.ActionGet, a.RBACName(s.ns)) { + // do not emit apps user does not have accessing + return false + } + + return true +} + func (s *Server) Watch(q *application.ApplicationQuery, ws application.ApplicationService_WatchServer) error { appName := q.GetName() appNs := s.appNamespaceOrDefault(q.GetAppNamespace()) @@ -1006,20 +1030,8 @@ func (s *Server) Watch(q *application.ApplicationQuery, ws application.Applicati // sendIfPermitted is a helper to send the application to the client's streaming channel if the // caller has RBAC privileges permissions to view it sendIfPermitted := func(a appv1.Application, eventType watch.EventType) { - if len(projects) > 0 && !projects[a.Spec.GetProject()] { - return - } - - if appVersion, err := strconv.Atoi(a.ResourceVersion); err == nil && appVersion < minVersion { - return - } - matchedEvent := (appName == "" || (a.Name == appName && a.Namespace == appNs)) && selector.Matches(labels.Set(a.Labels)) - if !matchedEvent { - return - } - - if !s.enf.Enforce(claims, rbacpolicy.ResourceApplications, rbacpolicy.ActionGet, a.RBACName(s.ns)) { - // do not emit apps user does not have accessing + permitted := s.isApplicationPermitted(selector, minVersion, claims, appName, appNs, projects, a) + if !permitted { return } s.inferResourcesStatusHealth(&a) diff --git a/server/application/application_test.go b/server/application/application_test.go index 7b8526908080f..346b1cf128190 100644 --- a/server/application/application_test.go +++ b/server/application/application_test.go @@ -10,6 +10,8 @@ import ( "testing" "time" + "k8s.io/apimachinery/pkg/labels" + "github.com/argoproj/gitops-engine/pkg/health" synccommon "github.com/argoproj/gitops-engine/pkg/sync/common" "github.com/argoproj/gitops-engine/pkg/utils/kube" @@ -1997,3 +1999,55 @@ func TestInferResourcesStatusHealth(t *testing.T) { assert.Equal(t, health.HealthStatusDegraded, testApp.Status.Resources[0].Health.Status) assert.Nil(t, testApp.Status.Resources[1].Health) } + +func TestIsApplicationPermitted(t *testing.T) { + t.Run("Incorrect project", func(t *testing.T) { + testApp := newTestApp() + appServer := newTestAppServer(t, testApp) + projects := map[string]bool{"test-app": false} + permitted := appServer.isApplicationPermitted(labels.Everything(), 0, nil, "test", "default", projects, *testApp) + assert.False(t, permitted) + }) + + t.Run("Version is incorrect", func(t *testing.T) { + testApp := newTestApp() + appServer := newTestAppServer(t, testApp) + minVersion := 100000 + testApp.ResourceVersion = strconv.Itoa(minVersion - 1) + permitted := appServer.isApplicationPermitted(labels.Everything(), minVersion, nil, "test", "default", nil, *testApp) + assert.False(t, permitted) + }) + + t.Run("Application name is incorrect", func(t *testing.T) { + testApp := newTestApp() + appServer := newTestAppServer(t, testApp) + appName := "test" + permitted := appServer.isApplicationPermitted(labels.Everything(), 0, nil, appName, "default", nil, *testApp) + assert.False(t, permitted) + }) + + t.Run("Application namespace is incorrect", func(t *testing.T) { + testApp := newTestApp() + appServer := newTestAppServer(t, testApp) + permitted := appServer.isApplicationPermitted(labels.Everything(), 0, nil, testApp.Name, "demo", nil, *testApp) + assert.False(t, permitted) + }) + + t.Run("Application is not part of enabled namespace", func(t *testing.T) { + testApp := newTestApp() + appServer := newTestAppServer(t, testApp) + appServer.ns = "server-ns" + appServer.enabledNamespaces = []string{"demo"} + permitted := appServer.isApplicationPermitted(labels.Everything(), 0, nil, testApp.Name, testApp.Namespace, nil, *testApp) + assert.False(t, permitted) + }) + + t.Run("Application is part of enabled namespace", func(t *testing.T) { + testApp := newTestApp() + appServer := newTestAppServer(t, testApp) + appServer.ns = "server-ns" + appServer.enabledNamespaces = []string{testApp.Namespace} + permitted := appServer.isApplicationPermitted(labels.Everything(), 0, nil, testApp.Name, testApp.Namespace, nil, *testApp) + assert.True(t, permitted) + }) +}