From b0480cbbc637a46ac8e0e438458b1d2e360b8681 Mon Sep 17 00:00:00 2001 From: pashakostohrys Date: Wed, 12 Jul 2023 18:19:38 +0300 Subject: [PATCH 1/4] fix: not need send application if it is not under enabled namespaces --- server/application/application.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/server/application/application.go b/server/application/application.go index 0a82be5f2f35c..87736418795b3 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -50,7 +50,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" @@ -225,7 +224,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)) { @@ -1024,6 +1023,10 @@ func (s *Server) Watch(q *application.ApplicationQuery, ws application.Applicati return } + if s.isNamespaceEnabled(a.Namespace) { + return + } + if !s.enf.Enforce(claims, rbacpolicy.ResourceApplications, rbacpolicy.ActionGet, a.RBACName(s.ns)) { // do not emit apps user does not have accessing return From 8d2e085c1ed40e6a98875a81782861886cd314b8 Mon Sep 17 00:00:00 2001 From: pashakostohrys Date: Wed, 12 Jul 2023 20:26:25 +0300 Subject: [PATCH 2/4] fix condition --- server/application/application.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/application/application.go b/server/application/application.go index 87736418795b3..96e85f2fa1d8c 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -224,7 +224,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 s.isNamespaceEnabled(a.Namespace) { + if !s.isNamespaceEnabled(a.Namespace) { continue } if s.enf.Enforce(ctx.Value("claims"), rbacpolicy.ResourceApplications, rbacpolicy.ActionGet, a.RBACName(s.ns)) { @@ -1023,7 +1023,7 @@ func (s *Server) Watch(q *application.ApplicationQuery, ws application.Applicati return } - if s.isNamespaceEnabled(a.Namespace) { + if !s.isNamespaceEnabled(a.Namespace) { return } From 73209b35d6c264e888320a66758f7e44eff46ebe Mon Sep 17 00:00:00 2001 From: pashakostohrys Date: Thu, 13 Jul 2023 15:05:30 +0300 Subject: [PATCH 3/4] feat: Move application is permitted outside of watch function and cover with unit tests --- server/application/application.go | 45 +++++++++++++--------- server/application/application_test.go | 53 ++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 18 deletions(-) diff --git a/server/application/application.go b/server/application/application.go index 96e85f2fa1d8c..ef35d76e84208 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -985,6 +985,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()) @@ -1011,24 +1036,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.isNamespaceEnabled(a.Namespace) { - 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 2dcefc121dfca..4b9e47957bf12 100644 --- a/server/application/application_test.go +++ b/server/application/application_test.go @@ -5,6 +5,7 @@ import ( coreerrors "errors" "fmt" "io" + "k8s.io/apimachinery/pkg/labels" "strconv" "sync/atomic" "testing" @@ -2202,3 +2203,55 @@ func TestRunOldStyleResourceAction(t *testing.T) { assert.NotNil(t, appResponse) }) } + +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, testApp.Namespace, 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) + }) +} From 133b18550c42892614e4b131ccafa0164c5bccf5 Mon Sep 17 00:00:00 2001 From: pashakostohrys Date: Thu, 13 Jul 2023 15:56:12 +0300 Subject: [PATCH 4/4] feat: Move application is permitted outside of watch function and cover with unit tests --- server/application/application_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/application/application_test.go b/server/application/application_test.go index 4b9e47957bf12..c78894a3f135b 100644 --- a/server/application/application_test.go +++ b/server/application/application_test.go @@ -2233,7 +2233,7 @@ func TestIsApplicationPermitted(t *testing.T) { 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, testApp.Namespace, nil, *testApp) + permitted := appServer.isApplicationPermitted(labels.Everything(), 0, nil, testApp.Name, "demo", nil, *testApp) assert.False(t, permitted) })