From 93c2d887948c9cfb6ed28599bcd2628ccfc67fa7 Mon Sep 17 00:00:00 2001 From: Sergey Smolnikov Date: Thu, 14 Sep 2023 17:14:55 +0200 Subject: [PATCH 01/12] Added validation to create a config map --- .vscode/launch.json | 1 + api/applications/applications_handler.go | 30 +++++++++++++++++++++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 65367744..a25b774a 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -20,6 +20,7 @@ "RADIX_ACTIVE_CLUSTER_EGRESS_IPS": "104.45.84.1", "REQUIRE_APP_CONFIGURATION_ITEM": "true", "REQUIRE_APP_AD_GROUPS": "true", + "RADIX_API_NAMESPACE": "radix-api-qa" }, "args": [ "--useOutClusterClient=false" diff --git a/api/applications/applications_handler.go b/api/applications/applications_handler.go index 960e7f86..2fd9110e 100644 --- a/api/applications/applications_handler.go +++ b/api/applications/applications_handler.go @@ -4,8 +4,8 @@ import ( "context" "encoding/json" "fmt" - "k8s.io/client-go/kubernetes/fake" "net/http" + "os" "strings" "time" @@ -33,6 +33,7 @@ import ( k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/fake" ) type patch struct { @@ -47,6 +48,7 @@ type ApplicationHandler struct { environmentHandler environments.EnvironmentHandler accounts models.Accounts config ApplicationHandlerConfig + namespace string } // NewApplicationHandler Constructor @@ -56,7 +58,18 @@ func NewApplicationHandler(accounts models.Accounts, config ApplicationHandlerCo jobHandler: job.Init(accounts, deployments.Init(accounts)), environmentHandler: environments.Init(environments.WithAccounts(accounts)), config: config, + namespace: getApiNamespace(), + } +} + +func getApiNamespace() string { + if buff, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err == nil { + return string(buff) + } + if namespace := os.Getenv("RADIX_API_NAMESPACE"); len(namespace) > 0 { + return namespace } + panic("radix-api namespace not found in downwardAPI or environment variable") } func (ah *ApplicationHandler) getUserAccount() models.Account { @@ -284,6 +297,9 @@ func (ah *ApplicationHandler) ModifyRegistrationDetails(ctx context.Context, app // Only these fields can change over time patchRequest := applicationRegistrationPatchRequest.ApplicationRegistrationPatch if patchRequest.AdGroups != nil && !radixutils.ArrayEqualElements(currentRegistration.Spec.AdGroups, *patchRequest.AdGroups) { + if err := ah.validateUserIsMemberOfAdGroups(patchRequest.AdGroups); err != nil { + return nil, radixhttp.ValidationError("Radix Registration", "User is not member of all AD groups") + } updatedRegistration.Spec.AdGroups = *patchRequest.AdGroups payload = append(payload, patch{Op: "replace", Path: "/spec/adGroups", Value: *patchRequest.AdGroups}) runUpdate = true @@ -727,6 +743,18 @@ func (ah *ApplicationHandler) userIsAppAdmin(ctx context.Context, appName string } } +func (ah *ApplicationHandler) validateUserIsMemberOfAdGroups(adGroups *[]string) error { + if len(*adGroups) == 0 { + return nil + } + + name := radixutils.RandString(10) + _, err := ah.accounts.UserAccount.Client.CoreV1().ConfigMaps(ah.namespace).Create(context.Background(), &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + }, metav1.CreateOptions{DryRun: []string{"All"}}) + return err +} + func setConfigBranchToFallbackWhenEmpty(existingRegistration *v1.RadixRegistration) bool { // HACK ConfigBranch is required, so we set it to "master" if empty to support existing apps registered before ConfigBranch was introduced if len(strings.TrimSpace(existingRegistration.Spec.ConfigBranch)) > 0 { From cce371a81d1db3f40c1e2e4a93f3ebebde55856f Mon Sep 17 00:00:00 2001 From: Sergey Smolnikov Date: Fri, 15 Sep 2023 16:58:08 +0200 Subject: [PATCH 02/12] Validate creating a config map --- .vscode/launch.json | 3 +- api/applications/applications_handler.go | 50 ++++++++----- .../applications_handler_config.go | 8 +- api/utils/rolebindings/rolebindings.go | 75 +++++++++++++++++++ api/utils/roles/roles.go | 36 +++++++++ 5 files changed, 151 insertions(+), 21 deletions(-) create mode 100644 api/utils/rolebindings/rolebindings.go create mode 100644 api/utils/roles/roles.go diff --git a/.vscode/launch.json b/.vscode/launch.json index a25b774a..ed088e52 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -20,7 +20,8 @@ "RADIX_ACTIVE_CLUSTER_EGRESS_IPS": "104.45.84.1", "REQUIRE_APP_CONFIGURATION_ITEM": "true", "REQUIRE_APP_AD_GROUPS": "true", - "RADIX_API_NAMESPACE": "radix-api-qa" + "RADIX_ENVIRONMENT":"qa", + "RADIX_APP":"radix-api" }, "args": [ "--useOutClusterClient=false" diff --git a/api/applications/applications_handler.go b/api/applications/applications_handler.go index 2fd9110e..f4e0e3b1 100644 --- a/api/applications/applications_handler.go +++ b/api/applications/applications_handler.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" "net/http" - "os" "strings" "time" @@ -17,6 +16,8 @@ import ( "github.com/equinor/radix-api/api/kubequery" apimodels "github.com/equinor/radix-api/api/models" "github.com/equinor/radix-api/api/utils" + "github.com/equinor/radix-api/api/utils/rolebindings" + "github.com/equinor/radix-api/api/utils/roles" "github.com/equinor/radix-api/models" radixhttp "github.com/equinor/radix-common/net/http" radixutils "github.com/equinor/radix-common/utils" @@ -58,18 +59,15 @@ func NewApplicationHandler(accounts models.Accounts, config ApplicationHandlerCo jobHandler: job.Init(accounts, deployments.Init(accounts)), environmentHandler: environments.Init(environments.WithAccounts(accounts)), config: config, - namespace: getApiNamespace(), + namespace: getApiNamespace(config), } } -func getApiNamespace() string { - if buff, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err == nil { - return string(buff) - } - if namespace := os.Getenv("RADIX_API_NAMESPACE"); len(namespace) > 0 { +func getApiNamespace(config ApplicationHandlerConfig) string { + if namespace := crdUtils.GetEnvironmentNamespace(config.AppName, config.EnvironmentName); len(namespace) > 0 { return namespace } - panic("radix-api namespace not found in downwardAPI or environment variable") + panic("missing RADIX_APP or RADIX_ENVIRONMENT environment variables") } func (ah *ApplicationHandler) getUserAccount() models.Account { @@ -290,15 +288,19 @@ func (ah *ApplicationHandler) ModifyRegistrationDetails(ctx context.Context, app return nil, err } - payload := []patch{} + payload := make([]patch, 0) runUpdate := false updatedRegistration := currentRegistration.DeepCopy() // Only these fields can change over time patchRequest := applicationRegistrationPatchRequest.ApplicationRegistrationPatch if patchRequest.AdGroups != nil && !radixutils.ArrayEqualElements(currentRegistration.Spec.AdGroups, *patchRequest.AdGroups) { - if err := ah.validateUserIsMemberOfAdGroups(patchRequest.AdGroups); err != nil { - return nil, radixhttp.ValidationError("Radix Registration", "User is not member of all AD groups") + err, valid := ah.validateUserIsMemberOfAdGroups(ctx, appName, patchRequest.AdGroups) + if err != nil { + return nil, err + } + if !valid { + return nil, radixhttp.ValidationError("Radix Registration", "User should be a member of at least one admin AD group or their sub-members") } updatedRegistration.Spec.AdGroups = *patchRequest.AdGroups payload = append(payload, patch{Op: "replace", Path: "/spec/adGroups", Value: *patchRequest.AdGroups}) @@ -570,7 +572,7 @@ func (ah *ApplicationHandler) getRegistrationUpdateWarnings(radixRegistration *v } func (ah *ApplicationHandler) isValidRegistrationInsert(radixRegistration *v1.RadixRegistration) error { - // Need to use in-cluster client of the API server, because the user might not have enough priviledges + // Need to use in-cluster client of the API server, because the user might not have enough privileges // to run a full validation return radixvalidators.CanRadixRegistrationBeInserted(ah.getServiceAccount().RadixClient, radixRegistration, ah.getAdditionalRadixRegistrationInsertValidators()...) } @@ -743,16 +745,28 @@ func (ah *ApplicationHandler) userIsAppAdmin(ctx context.Context, appName string } } -func (ah *ApplicationHandler) validateUserIsMemberOfAdGroups(adGroups *[]string) error { +func (ah *ApplicationHandler) validateUserIsMemberOfAdGroups(ctx context.Context, appName string, adGroups *[]string) (error, bool) { if len(*adGroups) == 0 { - return nil + return nil, false } - - name := radixutils.RandString(10) - _, err := ah.accounts.UserAccount.Client.CoreV1().ConfigMaps(ah.namespace).Create(context.Background(), &corev1.ConfigMap{ + name := fmt.Sprintf("access-validation-%s", appName) + defer func() { + _ = roles.DeleteRole(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, name) + _ = rolebindings.DeleteRoleBinding(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, name) + }() + err := roles.EnsureRoleCreateConfigMapExists(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, name, name) + if err != nil { + return err, false + } + err = rolebindings.EnsureRoleBindingExists(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, name, name, adGroups) + if err != nil { + return err, false + } + // dry-run of creating a config map + _, err = ah.accounts.UserAccount.Client.CoreV1().ConfigMaps(ah.namespace).Create(context.Background(), &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{Name: name}, }, metav1.CreateOptions{DryRun: []string{"All"}}) - return err + return nil, err == nil } func setConfigBranchToFallbackWhenEmpty(existingRegistration *v1.RadixRegistration) bool { diff --git a/api/applications/applications_handler_config.go b/api/applications/applications_handler_config.go index f8abffce..cbb74b58 100644 --- a/api/applications/applications_handler_config.go +++ b/api/applications/applications_handler_config.go @@ -27,8 +27,10 @@ func LoadApplicationHandlerConfig(args []string) (ApplicationHandlerConfig, erro } type ApplicationHandlerConfig struct { - RequireAppConfigurationItem bool `cfg:"require_app_configuration_item" flag:"require-app-configuration-item"` - RequireAppADGroups bool `cfg:"require_app_ad_groups" flag:"require-app-ad-groups"` + RequireAppConfigurationItem bool `cfg:"require_app_configuration_item" flag:"require-app-configuration-item"` + RequireAppADGroups bool `cfg:"require_app_ad_groups" flag:"require-app-ad-groups"` + AppName string `cfg:"radix_app" flag:"radix-app"` + EnvironmentName string `cfg:"radix_environment" flag:"radix-environment"` } func ApplicationHandlerConfigFlagSet() *pflag.FlagSet { @@ -36,5 +38,7 @@ func ApplicationHandlerConfigFlagSet() *pflag.FlagSet { flagset.ParseErrorsWhitelist = pflag.ParseErrorsWhitelist{UnknownFlags: true} flagset.Bool("require-app-configuration-item", true, "Require configuration item for application") flagset.Bool("require-app-ad-groups", true, "Require AD groups for application") + flagset.String("radix-app", "", "Application name") + flagset.String("radix-environment", "", "Environment name") return flagset } diff --git a/api/utils/rolebindings/rolebindings.go b/api/utils/rolebindings/rolebindings.go new file mode 100644 index 00000000..ee3dc052 --- /dev/null +++ b/api/utils/rolebindings/rolebindings.go @@ -0,0 +1,75 @@ +package rolebindings + +import ( + "context" + "encoding/json" + "fmt" + + "github.com/equinor/radix-operator/pkg/apis/defaults/k8s" + "github.com/equinor/radix-operator/pkg/apis/kube" + log "github.com/sirupsen/logrus" + rbacv1 "k8s.io/api/rbac/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/strategicpatch" + "k8s.io/client-go/kubernetes" +) + +// EnsureRoleBindingExists creates a role binding with a subject that binds AD groups to a specific role +func EnsureRoleBindingExists(ctx context.Context, kubeClient kubernetes.Interface, namespace, roleName, roleBindingName string, adGroups *[]string) error { + var subjects []rbacv1.Subject + for _, adGroup := range *adGroups { + subjects = append(subjects, rbacv1.Subject{ + Kind: k8s.KindGroup, + Name: adGroup, + APIGroup: k8s.RbacApiGroup, + }) + } + newRoleBinding := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{Name: roleBindingName}, + RoleRef: rbacv1.RoleRef{ + APIGroup: k8s.RbacApiGroup, + Kind: k8s.KindClusterRole, + Name: roleName, + }, Subjects: subjects, + } + existingRoleBinding, err := kubeClient.RbacV1().RoleBindings(namespace).Get(ctx, roleName, metav1.GetOptions{}) + if err != nil { + if k8serrors.IsNotFound(err) { + _, err = kubeClient.RbacV1().RoleBindings(namespace).Create(ctx, newRoleBinding, metav1.CreateOptions{}) + } + return err + } + + existingRoleBinding = existingRoleBinding.DeepCopy() + existingRoleBindingJSON, err := json.Marshal(existingRoleBinding) + if err != nil { + return fmt.Errorf("failed to marshal old role binding object: %v", err) + } + + newRoleBindingJSON, err := json.Marshal(newRoleBinding) + if err != nil { + return fmt.Errorf("failed to marshal new role binding object: %v", err) + } + + patchBytes, err := strategicpatch.CreateTwoWayMergePatch(existingRoleBindingJSON, newRoleBindingJSON, rbacv1.Role{}) + if err != nil { + return fmt.Errorf("failed to create two way merge patch role binding objects: %v", err) + } + + if kube.IsEmptyPatch(patchBytes) { + return nil + } + _, err = kubeClient.RbacV1().RoleBindings(namespace).Patch(ctx, roleBindingName, types.StrategicMergePatchType, patchBytes, metav1.PatchOptions{}) + if err != nil { + return fmt.Errorf("failed to patch role binding object: %v", err) + } + log.Debugf("Patched role binding: %s in namespace %s", roleBindingName, namespace) + return nil +} + +// DeleteRoleBinding Deletes a role binding +func DeleteRoleBinding(ctx context.Context, kubeClient kubernetes.Interface, namespace, roleBindingName string) error { + return kubeClient.RbacV1().RoleBindings(namespace).Delete(ctx, roleBindingName, metav1.DeleteOptions{}) +} diff --git a/api/utils/roles/roles.go b/api/utils/roles/roles.go new file mode 100644 index 00000000..70450910 --- /dev/null +++ b/api/utils/roles/roles.go @@ -0,0 +1,36 @@ +package roles + +import ( + "context" + + rbacv1 "k8s.io/api/rbac/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" +) + +// EnsureRoleCreateConfigMapExists creates a role with a rule that allows creating a configmap with the given name +func EnsureRoleCreateConfigMapExists(ctx context.Context, kubeClient kubernetes.Interface, namespace, roleName, configMapName string) error { + _, err := kubeClient.RbacV1().Roles(namespace).Get(ctx, roleName, metav1.GetOptions{}) + if err == nil { + return nil + } + if !k8serrors.IsNotFound(err) { + return err + } + _, err = kubeClient.RbacV1().Roles(namespace).Create(ctx, &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{Name: roleName}, + Rules: []rbacv1.PolicyRule{{ + Verbs: []string{"create"}, + APIGroups: []string{""}, + Resources: []string{"configmaps"}, + ResourceNames: []string{configMapName}, + }}, + }, metav1.CreateOptions{}) + return err +} + +// DeleteRole Deletes a role +func DeleteRole(ctx context.Context, kubeClient kubernetes.Interface, namespace, roleName string) error { + return kubeClient.RbacV1().Roles(namespace).Delete(ctx, roleName, metav1.DeleteOptions{}) +} From 5b2417d7eebf14b66548b6f16dd33f2bf5900038 Mon Sep 17 00:00:00 2001 From: Sergey Smolnikov Date: Wed, 20 Sep 2023 15:32:31 +0200 Subject: [PATCH 03/12] Validate get a config map --- .../applications_controller_test.go | 10 +- api/applications/applications_handler.go | 100 ++++++++++++++---- api/applications/get_applications_handler.go | 29 ++--- api/utils/access/access.go | 29 +++++ api/utils/rolebindings/rolebindings.go | 75 ------------- api/utils/roles/roles.go | 36 ------- 6 files changed, 124 insertions(+), 155 deletions(-) create mode 100644 api/utils/access/access.go delete mode 100644 api/utils/rolebindings/rolebindings.go delete mode 100644 api/utils/roles/roles.go diff --git a/api/applications/applications_controller_test.go b/api/applications/applications_controller_test.go index 1f3bcb1b..a491ca6e 100644 --- a/api/applications/applications_controller_test.go +++ b/api/applications/applications_controller_test.go @@ -91,7 +91,7 @@ func TestGetApplications_HasAccessToSomeRR(t *testing.T) { NewApplicationController( func(_ context.Context, _ kubernetes.Interface, _ v1.RadixRegistration) (bool, error) { return false, nil - }, NewApplicationHandlerFactory(ApplicationHandlerConfig{true, true}))) + }, NewApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}))) responseChannel := controllerTestUtils.ExecuteRequest("GET", "/api/v1/applications") response := <-responseChannel @@ -104,7 +104,7 @@ func TestGetApplications_HasAccessToSomeRR(t *testing.T) { controllerTestUtils := controllertest.NewTestUtils(kubeclient, radixclient, secretproviderclient, NewApplicationController( func(_ context.Context, _ kubernetes.Interface, rr v1.RadixRegistration) (bool, error) { return rr.GetName() == "my-second-app", nil - }, NewApplicationHandlerFactory(ApplicationHandlerConfig{true, true}))) + }, NewApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}))) responseChannel := controllerTestUtils.ExecuteRequest("GET", "/api/v1/applications") response := <-responseChannel @@ -117,7 +117,7 @@ func TestGetApplications_HasAccessToSomeRR(t *testing.T) { controllerTestUtils := controllertest.NewTestUtils(kubeclient, radixclient, secretproviderclient, NewApplicationController( func(_ context.Context, _ kubernetes.Interface, _ v1.RadixRegistration) (bool, error) { return true, nil - }, NewApplicationHandlerFactory(ApplicationHandlerConfig{true, true}))) + }, NewApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}))) responseChannel := controllerTestUtils.ExecuteRequest("GET", "/api/v1/applications") response := <-responseChannel @@ -185,7 +185,7 @@ func TestSearchApplications(t *testing.T) { controllerTestUtils := controllertest.NewTestUtils(kubeclient, radixclient, secretproviderclient, NewApplicationController( func(_ context.Context, _ kubernetes.Interface, _ v1.RadixRegistration) (bool, error) { return true, nil - }, NewApplicationHandlerFactory(ApplicationHandlerConfig{true, true}))) + }, NewApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}))) // Tests t.Run("search for "+appNames[0], func(t *testing.T) { @@ -259,7 +259,7 @@ func TestSearchApplications(t *testing.T) { controllerTestUtils := controllertest.NewTestUtils(kubeclient, radixclient, secretproviderclient, NewApplicationController( func(_ context.Context, _ kubernetes.Interface, _ v1.RadixRegistration) (bool, error) { return false, nil - }, NewApplicationHandlerFactory(ApplicationHandlerConfig{true, true}))) + }, NewApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}))) searchParam := applicationModels.ApplicationsSearchRequest{Names: []string{appNames[0]}} responseChannel := controllerTestUtils.ExecuteRequestWithParameters("POST", "/api/v1/applications/_search", &searchParam) response := <-responseChannel diff --git a/api/applications/applications_handler.go b/api/applications/applications_handler.go index f4e0e3b1..2c561cab 100644 --- a/api/applications/applications_handler.go +++ b/api/applications/applications_handler.go @@ -16,24 +16,27 @@ import ( "github.com/equinor/radix-api/api/kubequery" apimodels "github.com/equinor/radix-api/api/models" "github.com/equinor/radix-api/api/utils" - "github.com/equinor/radix-api/api/utils/rolebindings" - "github.com/equinor/radix-api/api/utils/roles" + "github.com/equinor/radix-api/api/utils/access" "github.com/equinor/radix-api/models" radixhttp "github.com/equinor/radix-common/net/http" radixutils "github.com/equinor/radix-common/utils" "github.com/equinor/radix-common/utils/slice" "github.com/equinor/radix-operator/pkg/apis/applicationconfig" "github.com/equinor/radix-operator/pkg/apis/defaults" + "github.com/equinor/radix-operator/pkg/apis/defaults/k8s" jobPipeline "github.com/equinor/radix-operator/pkg/apis/pipeline" v1 "github.com/equinor/radix-operator/pkg/apis/radix/v1" "github.com/equinor/radix-operator/pkg/apis/radixvalidators" crdUtils "github.com/equinor/radix-operator/pkg/apis/utils" log "github.com/sirupsen/logrus" + authorizationapi "k8s.io/api/authorization/v1" corev1auth "k8s.io/api/authorization/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" ) @@ -295,7 +298,7 @@ func (ah *ApplicationHandler) ModifyRegistrationDetails(ctx context.Context, app // Only these fields can change over time patchRequest := applicationRegistrationPatchRequest.ApplicationRegistrationPatch if patchRequest.AdGroups != nil && !radixutils.ArrayEqualElements(currentRegistration.Spec.AdGroups, *patchRequest.AdGroups) { - err, valid := ah.validateUserIsMemberOfAdGroups(ctx, appName, patchRequest.AdGroups) + valid, err := ah.validateUserIsMemberOfAdGroups(ctx, appName, patchRequest.AdGroups) if err != nil { return nil, err } @@ -745,28 +748,35 @@ func (ah *ApplicationHandler) userIsAppAdmin(ctx context.Context, appName string } } -func (ah *ApplicationHandler) validateUserIsMemberOfAdGroups(ctx context.Context, appName string, adGroups *[]string) (error, bool) { +func (ah *ApplicationHandler) validateUserIsMemberOfAdGroups(ctx context.Context, appName string, adGroups *[]string) (bool, error) { if len(*adGroups) == 0 { - return nil, false + return false, nil } name := fmt.Sprintf("access-validation-%s", appName) - defer func() { - _ = roles.DeleteRole(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, name) - _ = rolebindings.DeleteRoleBinding(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, name) - }() - err := roles.EnsureRoleCreateConfigMapExists(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, name, name) + labels := map[string]string{"radix-access-validation": "true"} + configMapName := fmt.Sprintf("%s-%s", name, crdUtils.RandString(6)) + role, err := createRoleToGetConfigMap(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, name, labels, configMapName) if err != nil { - return err, false + return false, err } - err = rolebindings.EnsureRoleBindingExists(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, name, name, adGroups) + defer func() { + _ = deleteRole(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, role.GetName()) + }() + roleBinding, err := createRoleBindingForRole(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, role, name, adGroups, labels) if err != nil { - return err, false + return false, err } - // dry-run of creating a config map - _, err = ah.accounts.UserAccount.Client.CoreV1().ConfigMaps(ah.namespace).Create(context.Background(), &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: name}, - }, metav1.CreateOptions{DryRun: []string{"All"}}) - return nil, err == nil + defer func() { + _ = deleteRoleBinding(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, roleBinding.GetName()) + }() + + return access.HasAccess(ctx, ah.accounts.UserAccount.Client, &authorizationapi.ResourceAttributes{ + Verb: "get", + Group: "", + Resource: "configmaps", + Version: "*", + Name: configMapName, + }) } func setConfigBranchToFallbackWhenEmpty(existingRegistration *v1.RadixRegistration) bool { @@ -777,3 +787,57 @@ func setConfigBranchToFallbackWhenEmpty(existingRegistration *v1.RadixRegistrati existingRegistration.Spec.ConfigBranch = applicationconfig.ConfigBranchFallback return true } + +func createRoleToGetConfigMap(ctx context.Context, kubeClient kubernetes.Interface, namespace, roleName string, labels map[string]string, configMapName string) (*rbacv1.Role, error) { + return kubeClient.RbacV1().Roles(namespace).Create(ctx, &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{GenerateName: roleName, Labels: labels}, + Rules: []rbacv1.PolicyRule{{ + Verbs: []string{"get"}, + APIGroups: []string{""}, + Resources: []string{"configmaps"}, + ResourceNames: []string{configMapName}, + }}, + }, metav1.CreateOptions{}) +} + +func deleteRole(ctx context.Context, kubeClient kubernetes.Interface, namespace, roleName string) error { + deletionPropagation := metav1.DeletePropagationBackground + return kubeClient.RbacV1().Roles(namespace).Delete(ctx, roleName, metav1.DeleteOptions{ + PropagationPolicy: &deletionPropagation, + }) +} + +func createRoleBindingForRole(ctx context.Context, kubeClient kubernetes.Interface, namespace string, role *rbacv1.Role, roleBindingName string, adGroups *[]string, labels map[string]string) (*rbacv1.RoleBinding, error) { + var subjects []rbacv1.Subject + for _, adGroup := range *adGroups { + subjects = append(subjects, rbacv1.Subject{ + Kind: k8s.KindGroup, + Name: adGroup, + APIGroup: k8s.RbacApiGroup, + }) + } + newRoleBinding := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: roleBindingName, + Labels: labels, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: k8s.RbacApiVersion, + Kind: k8s.KindRole, + Name: role.GetName(), + UID: role.GetUID(), + }, + }, + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: k8s.RbacApiGroup, + Kind: k8s.KindRole, + Name: role.GetName(), + }, Subjects: subjects, + } + return kubeClient.RbacV1().RoleBindings(namespace).Create(ctx, newRoleBinding, metav1.CreateOptions{}) +} + +func deleteRoleBinding(ctx context.Context, kubeClient kubernetes.Interface, namespace, roleBindingName string) error { + return kubeClient.RbacV1().RoleBindings(namespace).Delete(ctx, roleBindingName, metav1.DeleteOptions{}) +} diff --git a/api/applications/get_applications_handler.go b/api/applications/get_applications_handler.go index ead6ab67..2b5eb56e 100644 --- a/api/applications/get_applications_handler.go +++ b/api/applications/get_applications_handler.go @@ -10,6 +10,7 @@ import ( deploymentModels "github.com/equinor/radix-api/api/deployments/models" environmentModels "github.com/equinor/radix-api/api/environments/models" jobModels "github.com/equinor/radix-api/api/jobs/models" + "github.com/equinor/radix-api/api/utils/access" "golang.org/x/sync/errgroup" authorizationapi "k8s.io/api/authorization/v1" @@ -212,25 +213,11 @@ func (ah *ApplicationHandler) filterRadixRegByAccess(ctx context.Context, radixr // cannot run as test - does not return correct values func hasAccess(ctx context.Context, client kubernetes.Interface, rr v1.RadixRegistration) (bool, error) { - sar := authorizationapi.SelfSubjectAccessReview{ - Spec: authorizationapi.SelfSubjectAccessReviewSpec{ - ResourceAttributes: &authorizationapi.ResourceAttributes{ - Verb: "get", - Group: "radix.equinor.com", - Resource: "radixregistrations", - Version: "*", - Name: rr.GetName(), - }, - }, - } - - r, err := postSelfSubjectAccessReviews(ctx, client, sar) - if err != nil { - return false, err - } - return r.Status.Allowed, nil -} - -func postSelfSubjectAccessReviews(ctx context.Context, client kubernetes.Interface, sar authorizationapi.SelfSubjectAccessReview) (*authorizationapi.SelfSubjectAccessReview, error) { - return client.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, &sar, metav1.CreateOptions{}) + return access.HasAccess(ctx, client, &authorizationapi.ResourceAttributes{ + Verb: "get", + Group: "radix.equinor.com", + Resource: "radixregistrations", + Version: "*", + Name: rr.GetName(), + }) } diff --git a/api/utils/access/access.go b/api/utils/access/access.go new file mode 100644 index 00000000..e642db00 --- /dev/null +++ b/api/utils/access/access.go @@ -0,0 +1,29 @@ +package access + +import ( + "context" + + authorizationapi "k8s.io/api/authorization/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" +) + +// HasAccess checks if user has access to a resource +// cannot run as test - does not return correct values +func HasAccess(ctx context.Context, client kubernetes.Interface, resourceAttributes *authorizationapi.ResourceAttributes) (bool, error) { + sar := authorizationapi.SelfSubjectAccessReview{ + Spec: authorizationapi.SelfSubjectAccessReviewSpec{ + ResourceAttributes: resourceAttributes, + }, + } + + r, err := postSelfSubjectAccessReviews(ctx, client, sar) + if err != nil { + return false, err + } + return r.Status.Allowed, nil +} + +func postSelfSubjectAccessReviews(ctx context.Context, client kubernetes.Interface, sar authorizationapi.SelfSubjectAccessReview) (*authorizationapi.SelfSubjectAccessReview, error) { + return client.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, &sar, metav1.CreateOptions{}) +} diff --git a/api/utils/rolebindings/rolebindings.go b/api/utils/rolebindings/rolebindings.go deleted file mode 100644 index ee3dc052..00000000 --- a/api/utils/rolebindings/rolebindings.go +++ /dev/null @@ -1,75 +0,0 @@ -package rolebindings - -import ( - "context" - "encoding/json" - "fmt" - - "github.com/equinor/radix-operator/pkg/apis/defaults/k8s" - "github.com/equinor/radix-operator/pkg/apis/kube" - log "github.com/sirupsen/logrus" - rbacv1 "k8s.io/api/rbac/v1" - k8serrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/strategicpatch" - "k8s.io/client-go/kubernetes" -) - -// EnsureRoleBindingExists creates a role binding with a subject that binds AD groups to a specific role -func EnsureRoleBindingExists(ctx context.Context, kubeClient kubernetes.Interface, namespace, roleName, roleBindingName string, adGroups *[]string) error { - var subjects []rbacv1.Subject - for _, adGroup := range *adGroups { - subjects = append(subjects, rbacv1.Subject{ - Kind: k8s.KindGroup, - Name: adGroup, - APIGroup: k8s.RbacApiGroup, - }) - } - newRoleBinding := &rbacv1.RoleBinding{ - ObjectMeta: metav1.ObjectMeta{Name: roleBindingName}, - RoleRef: rbacv1.RoleRef{ - APIGroup: k8s.RbacApiGroup, - Kind: k8s.KindClusterRole, - Name: roleName, - }, Subjects: subjects, - } - existingRoleBinding, err := kubeClient.RbacV1().RoleBindings(namespace).Get(ctx, roleName, metav1.GetOptions{}) - if err != nil { - if k8serrors.IsNotFound(err) { - _, err = kubeClient.RbacV1().RoleBindings(namespace).Create(ctx, newRoleBinding, metav1.CreateOptions{}) - } - return err - } - - existingRoleBinding = existingRoleBinding.DeepCopy() - existingRoleBindingJSON, err := json.Marshal(existingRoleBinding) - if err != nil { - return fmt.Errorf("failed to marshal old role binding object: %v", err) - } - - newRoleBindingJSON, err := json.Marshal(newRoleBinding) - if err != nil { - return fmt.Errorf("failed to marshal new role binding object: %v", err) - } - - patchBytes, err := strategicpatch.CreateTwoWayMergePatch(existingRoleBindingJSON, newRoleBindingJSON, rbacv1.Role{}) - if err != nil { - return fmt.Errorf("failed to create two way merge patch role binding objects: %v", err) - } - - if kube.IsEmptyPatch(patchBytes) { - return nil - } - _, err = kubeClient.RbacV1().RoleBindings(namespace).Patch(ctx, roleBindingName, types.StrategicMergePatchType, patchBytes, metav1.PatchOptions{}) - if err != nil { - return fmt.Errorf("failed to patch role binding object: %v", err) - } - log.Debugf("Patched role binding: %s in namespace %s", roleBindingName, namespace) - return nil -} - -// DeleteRoleBinding Deletes a role binding -func DeleteRoleBinding(ctx context.Context, kubeClient kubernetes.Interface, namespace, roleBindingName string) error { - return kubeClient.RbacV1().RoleBindings(namespace).Delete(ctx, roleBindingName, metav1.DeleteOptions{}) -} diff --git a/api/utils/roles/roles.go b/api/utils/roles/roles.go deleted file mode 100644 index 70450910..00000000 --- a/api/utils/roles/roles.go +++ /dev/null @@ -1,36 +0,0 @@ -package roles - -import ( - "context" - - rbacv1 "k8s.io/api/rbac/v1" - k8serrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/kubernetes" -) - -// EnsureRoleCreateConfigMapExists creates a role with a rule that allows creating a configmap with the given name -func EnsureRoleCreateConfigMapExists(ctx context.Context, kubeClient kubernetes.Interface, namespace, roleName, configMapName string) error { - _, err := kubeClient.RbacV1().Roles(namespace).Get(ctx, roleName, metav1.GetOptions{}) - if err == nil { - return nil - } - if !k8serrors.IsNotFound(err) { - return err - } - _, err = kubeClient.RbacV1().Roles(namespace).Create(ctx, &rbacv1.Role{ - ObjectMeta: metav1.ObjectMeta{Name: roleName}, - Rules: []rbacv1.PolicyRule{{ - Verbs: []string{"create"}, - APIGroups: []string{""}, - Resources: []string{"configmaps"}, - ResourceNames: []string{configMapName}, - }}, - }, metav1.CreateOptions{}) - return err -} - -// DeleteRole Deletes a role -func DeleteRole(ctx context.Context, kubeClient kubernetes.Interface, namespace, roleName string) error { - return kubeClient.RbacV1().Roles(namespace).Delete(ctx, roleName, metav1.DeleteOptions{}) -} From 5b2c4e514137ed36f73e29610e9ac4ab7332bcd9 Mon Sep 17 00:00:00 2001 From: Sergey Smolnikov Date: Wed, 20 Sep 2023 16:53:23 +0200 Subject: [PATCH 04/12] Fixed deleting of temp role and rolebinding --- api/applications/applications_handler.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/applications/applications_handler.go b/api/applications/applications_handler.go index 2c561cab..ad921855 100644 --- a/api/applications/applications_handler.go +++ b/api/applications/applications_handler.go @@ -754,20 +754,20 @@ func (ah *ApplicationHandler) validateUserIsMemberOfAdGroups(ctx context.Context } name := fmt.Sprintf("access-validation-%s", appName) labels := map[string]string{"radix-access-validation": "true"} - configMapName := fmt.Sprintf("%s-%s", name, crdUtils.RandString(6)) + configMapName := fmt.Sprintf("%s-%s", name, strings.ToLower(crdUtils.RandString(6))) role, err := createRoleToGetConfigMap(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, name, labels, configMapName) if err != nil { return false, err } defer func() { - _ = deleteRole(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, role.GetName()) + _ = deleteRole(context.Background(), ah.accounts.ServiceAccount.Client, ah.namespace, role.GetName()) }() roleBinding, err := createRoleBindingForRole(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, role, name, adGroups, labels) if err != nil { return false, err } defer func() { - _ = deleteRoleBinding(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, roleBinding.GetName()) + _ = deleteRoleBinding(context.Background(), ah.accounts.ServiceAccount.Client, ah.namespace, roleBinding.GetName()) }() return access.HasAccess(ctx, ah.accounts.UserAccount.Client, &authorizationapi.ResourceAttributes{ From d69c5bf04a55b4ebc578227cf4bc0b09d2f23c48 Mon Sep 17 00:00:00 2001 From: Sergey Smolnikov Date: Thu, 21 Sep 2023 10:25:57 +0200 Subject: [PATCH 05/12] Fixed has access --- api/applications/applications_handler.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/api/applications/applications_handler.go b/api/applications/applications_handler.go index ad921855..48e1e3ee 100644 --- a/api/applications/applications_handler.go +++ b/api/applications/applications_handler.go @@ -771,11 +771,12 @@ func (ah *ApplicationHandler) validateUserIsMemberOfAdGroups(ctx context.Context }() return access.HasAccess(ctx, ah.accounts.UserAccount.Client, &authorizationapi.ResourceAttributes{ - Verb: "get", - Group: "", - Resource: "configmaps", - Version: "*", - Name: configMapName, + Verb: "get", + Group: "", + Resource: "configmaps", + Version: "*", + Namespace: ah.namespace, + Name: configMapName, }) } From 1a2a567aeea86d279f7f26d170a98cccbc56ca50 Mon Sep 17 00:00:00 2001 From: Sergey Smolnikov Date: Thu, 21 Sep 2023 10:35:13 +0200 Subject: [PATCH 06/12] Corrected prop name case --- api/applications/models/application_registration_patch.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/applications/models/application_registration_patch.go b/api/applications/models/application_registration_patch.go index 6003f0a6..cd0d3acc 100644 --- a/api/applications/models/application_registration_patch.go +++ b/api/applications/models/application_registration_patch.go @@ -11,7 +11,7 @@ type ApplicationRegistrationPatch struct { // ReaderAdGroups the groups that should be able to read the application // // required: false - ReaderAdGroups *[]string `json:"ReaderAdGroups,omitempty"` + ReaderAdGroups *[]string `json:"readerAdGroups,omitempty"` // Owner of the application - should be an email // From 216b455e95a151cb2f471488b5c7ff86a0106c23 Mon Sep 17 00:00:00 2001 From: Sergey Smolnikov Date: Fri, 22 Sep 2023 12:26:00 +0200 Subject: [PATCH 07/12] Added validation to create app method. Removed from playground case --- api/applications/applications_handler.go | 48 ++++++++++++++---------- api/applications/errors.go | 7 ++++ 2 files changed, 36 insertions(+), 19 deletions(-) create mode 100644 api/applications/errors.go diff --git a/api/applications/applications_handler.go b/api/applications/applications_handler.go index 48e1e3ee..582d3bdb 100644 --- a/api/applications/applications_handler.go +++ b/api/applications/applications_handler.go @@ -30,7 +30,6 @@ import ( crdUtils "github.com/equinor/radix-operator/pkg/apis/utils" log "github.com/sirupsen/logrus" authorizationapi "k8s.io/api/authorization/v1" - corev1auth "k8s.io/api/authorization/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" @@ -193,6 +192,10 @@ func (ah *ApplicationHandler) RegisterApplication(ctx context.Context, applicati return upsertResponse, err } } + err = ah.validateUserIsMemberOfAdGroups(ctx, applicationRegistrationRequest.ApplicationRegistration.Name, applicationRegistrationRequest.ApplicationRegistration.AdGroups) + if err != nil { + return nil, err + } radixRegistration, err = ah.getUserAccount().RadixClient.RadixV1().RadixRegistrations().Create(ctx, radixRegistration, metav1.CreateOptions{}) if err != nil { @@ -298,13 +301,6 @@ func (ah *ApplicationHandler) ModifyRegistrationDetails(ctx context.Context, app // Only these fields can change over time patchRequest := applicationRegistrationPatchRequest.ApplicationRegistrationPatch if patchRequest.AdGroups != nil && !radixutils.ArrayEqualElements(currentRegistration.Spec.AdGroups, *patchRequest.AdGroups) { - valid, err := ah.validateUserIsMemberOfAdGroups(ctx, appName, patchRequest.AdGroups) - if err != nil { - return nil, err - } - if !valid { - return nil, radixhttp.ValidationError("Radix Registration", "User should be a member of at least one admin AD group or their sub-members") - } updatedRegistration.Spec.AdGroups = *patchRequest.AdGroups payload = append(payload, patch{Op: "replace", Path: "/spec/adGroups", Value: *patchRequest.AdGroups}) runUpdate = true @@ -376,6 +372,10 @@ func (ah *ApplicationHandler) ModifyRegistrationDetails(ctx context.Context, app } if runUpdate { + err := ah.validateUserIsMemberOfAdGroups(ctx, appName, updatedRegistration.Spec.AdGroups) + if err != nil { + return nil, err + } err = ah.isValidRegistrationUpdate(updatedRegistration, currentRegistration) if err != nil { return nil, err @@ -734,9 +734,9 @@ func (ah *ApplicationHandler) userIsAppAdmin(ctx context.Context, appName string case *fake.Clientset: return true, nil default: - review, err := ah.accounts.UserAccount.Client.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, &corev1auth.SelfSubjectAccessReview{ - Spec: corev1auth.SelfSubjectAccessReviewSpec{ - ResourceAttributes: &corev1auth.ResourceAttributes{ + review, err := ah.accounts.UserAccount.Client.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, &authorizationapi.SelfSubjectAccessReview{ + Spec: authorizationapi.SelfSubjectAccessReviewSpec{ + ResourceAttributes: &authorizationapi.ResourceAttributes{ Verb: "patch", Group: "radix.equinor.com", Resource: "radixregistrations", @@ -748,29 +748,32 @@ func (ah *ApplicationHandler) userIsAppAdmin(ctx context.Context, appName string } } -func (ah *ApplicationHandler) validateUserIsMemberOfAdGroups(ctx context.Context, appName string, adGroups *[]string) (bool, error) { - if len(*adGroups) == 0 { - return false, nil +func (ah *ApplicationHandler) validateUserIsMemberOfAdGroups(ctx context.Context, appName string, adGroups []string) error { + if len(adGroups) == 0 { + if ah.config.RequireAppADGroups { + return userShouldBeMemberOfAdminAdGroupError() + } + return nil } name := fmt.Sprintf("access-validation-%s", appName) labels := map[string]string{"radix-access-validation": "true"} configMapName := fmt.Sprintf("%s-%s", name, strings.ToLower(crdUtils.RandString(6))) role, err := createRoleToGetConfigMap(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, name, labels, configMapName) if err != nil { - return false, err + return err } defer func() { _ = deleteRole(context.Background(), ah.accounts.ServiceAccount.Client, ah.namespace, role.GetName()) }() roleBinding, err := createRoleBindingForRole(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, role, name, adGroups, labels) if err != nil { - return false, err + return err } defer func() { _ = deleteRoleBinding(context.Background(), ah.accounts.ServiceAccount.Client, ah.namespace, roleBinding.GetName()) }() - return access.HasAccess(ctx, ah.accounts.UserAccount.Client, &authorizationapi.ResourceAttributes{ + valid, err := access.HasAccess(ctx, ah.accounts.UserAccount.Client, &authorizationapi.ResourceAttributes{ Verb: "get", Group: "", Resource: "configmaps", @@ -778,6 +781,13 @@ func (ah *ApplicationHandler) validateUserIsMemberOfAdGroups(ctx context.Context Namespace: ah.namespace, Name: configMapName, }) + if err != nil { + return err + } + if valid { + return nil + } + return userShouldBeMemberOfAdminAdGroupError() } func setConfigBranchToFallbackWhenEmpty(existingRegistration *v1.RadixRegistration) bool { @@ -808,9 +818,9 @@ func deleteRole(ctx context.Context, kubeClient kubernetes.Interface, namespace, }) } -func createRoleBindingForRole(ctx context.Context, kubeClient kubernetes.Interface, namespace string, role *rbacv1.Role, roleBindingName string, adGroups *[]string, labels map[string]string) (*rbacv1.RoleBinding, error) { +func createRoleBindingForRole(ctx context.Context, kubeClient kubernetes.Interface, namespace string, role *rbacv1.Role, roleBindingName string, adGroups []string, labels map[string]string) (*rbacv1.RoleBinding, error) { var subjects []rbacv1.Subject - for _, adGroup := range *adGroups { + for _, adGroup := range adGroups { subjects = append(subjects, rbacv1.Subject{ Kind: k8s.KindGroup, Name: adGroup, diff --git a/api/applications/errors.go b/api/applications/errors.go new file mode 100644 index 00000000..ace8f507 --- /dev/null +++ b/api/applications/errors.go @@ -0,0 +1,7 @@ +package applications + +import radixhttp "github.com/equinor/radix-common/net/http" + +func userShouldBeMemberOfAdminAdGroupError() error { + return radixhttp.ValidationError("Radix Registration", "User should be a member of at least one admin AD group or their sub-members") +} From 8cccf4571ce36a0df651dc221e9c96738f68f4a7 Mon Sep 17 00:00:00 2001 From: Sergey Smolnikov Date: Fri, 22 Sep 2023 12:47:57 +0200 Subject: [PATCH 08/12] Added warning --- api/applications/applications_handler.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/api/applications/applications_handler.go b/api/applications/applications_handler.go index 582d3bdb..43d30526 100644 --- a/api/applications/applications_handler.go +++ b/api/applications/applications_handler.go @@ -763,14 +763,20 @@ func (ah *ApplicationHandler) validateUserIsMemberOfAdGroups(ctx context.Context return err } defer func() { - _ = deleteRole(context.Background(), ah.accounts.ServiceAccount.Client, ah.namespace, role.GetName()) + err = deleteRole(context.Background(), ah.accounts.ServiceAccount.Client, ah.namespace, role.GetName()) + if err != nil { + log.Warnf("Failed to delete role %s: %v", role.GetName(), err) + } }() roleBinding, err := createRoleBindingForRole(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, role, name, adGroups, labels) if err != nil { return err } defer func() { - _ = deleteRoleBinding(context.Background(), ah.accounts.ServiceAccount.Client, ah.namespace, roleBinding.GetName()) + err = deleteRoleBinding(context.Background(), ah.accounts.ServiceAccount.Client, ah.namespace, roleBinding.GetName()) + if err != nil { + log.Warnf("Failed to delete role binding %s: %v", roleBinding.GetName(), err) + } }() valid, err := access.HasAccess(ctx, ah.accounts.UserAccount.Client, &authorizationapi.ResourceAttributes{ From 579ddedbe60fe5738582f8b9d409825ffc893386 Mon Sep 17 00:00:00 2001 From: Sergey Smolnikov Date: Fri, 22 Sep 2023 12:50:56 +0200 Subject: [PATCH 09/12] Corrected --- api/applications/applications_handler.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/applications/applications_handler.go b/api/applications/applications_handler.go index 43d30526..2f7c0f81 100644 --- a/api/applications/applications_handler.go +++ b/api/applications/applications_handler.go @@ -790,10 +790,10 @@ func (ah *ApplicationHandler) validateUserIsMemberOfAdGroups(ctx context.Context if err != nil { return err } - if valid { - return nil + if !valid { + return userShouldBeMemberOfAdminAdGroupError() } - return userShouldBeMemberOfAdminAdGroupError() + return nil } func setConfigBranchToFallbackWhenEmpty(existingRegistration *v1.RadixRegistration) bool { From edd057e38a48b22216553a847dd09f7e0d96bd19 Mon Sep 17 00:00:00 2001 From: Sergey Smolnikov Date: Fri, 22 Sep 2023 13:41:51 +0200 Subject: [PATCH 10/12] Fixed unit-tests --- api/applications/applications_controller.go | 30 ++++++------ .../applications_controller_test.go | 48 ++++++++++++++++--- api/applications/applications_handler.go | 44 ++++++++--------- .../applications_handler_factory.go | 32 +++++++++++-- 4 files changed, 106 insertions(+), 48 deletions(-) diff --git a/api/applications/applications_controller.go b/api/applications/applications_controller.go index e21dfcf0..89389964 100644 --- a/api/applications/applications_controller.go +++ b/api/applications/applications_controller.go @@ -178,7 +178,7 @@ func (ac *applicationController) ShowApplications(accounts models.Accounts, w ht matcher = applicationModels.MatchBySSHRepoFunc(sshRepo) } - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) appRegistrations, err := handler.GetApplications(r.Context(), matcher, ac.hasAccessToRR, GetApplicationsOptions{}) if err != nil { @@ -243,7 +243,7 @@ func (ac *applicationController) SearchApplications(accounts models.Accounts, w return } - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) matcher := applicationModels.MatchByNamesFunc(appNamesRequest.Names) appRegistrations, err := handler.GetApplications( @@ -304,7 +304,7 @@ func (ac *applicationController) GetApplication(accounts models.Accounts, w http appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) application, err := handler.GetApplication(r.Context(), appName) @@ -404,7 +404,7 @@ func (ac *applicationController) RegenerateMachineUserTokenHandler(accounts mode // description: "Internal server error" appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) machineUser, err := handler.RegenerateMachineUserToken(r.Context(), appName) if err != nil { @@ -453,7 +453,7 @@ func (ac *applicationController) RegenerateDeployKeyHandler(accounts models.Acco // "404": // description: "Not found" appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) var sharedSecretAndPrivateKey applicationModels.RegenerateDeployKeyAndSecretData if err := json.NewDecoder(r.Body).Decode(&sharedSecretAndPrivateKey); err != nil { radixhttp.ErrorResponse(w, r, err) @@ -501,7 +501,7 @@ func (ac *applicationController) GetDeployKeyAndSecret(accounts models.Accounts, // "404": // description: "Not found" appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) deployKeyAndSecret, err := handler.GetDeployKeyAndSecret(r.Context(), appName) if err != nil { @@ -555,7 +555,7 @@ func (ac *applicationController) RegisterApplication(accounts models.Accounts, w } // Need in cluster Radix client in order to validate registration using sufficient privileges - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) appRegistrationUpsertResponse, err := handler.RegisterApplication(r.Context(), applicationRegistrationRequest) if err != nil { radixhttp.ErrorResponse(w, r, err) @@ -616,7 +616,7 @@ func (ac *applicationController) ChangeRegistrationDetails(accounts models.Accou } // Need in cluster Radix client in order to validate registration using sufficient privileges - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) appRegistrationUpsertResponse, err := handler.ChangeRegistrationDetails(r.Context(), appName, applicationRegistrationRequest) if err != nil { radixhttp.ErrorResponse(w, r, err) @@ -677,7 +677,7 @@ func (ac *applicationController) ModifyRegistrationDetails(accounts models.Accou } // Need in cluster Radix client in order to validate registration using sufficient privileges - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) appRegistrationUpsertResponse, err := handler.ModifyRegistrationDetails(r.Context(), appName, applicationRegistrationPatchRequest) if err != nil { radixhttp.ErrorResponse(w, r, err) @@ -721,7 +721,7 @@ func (ac *applicationController) DeleteApplication(accounts models.Accounts, w h // description: "Not found" appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) err := handler.DeleteApplication(r.Context(), appName) if err != nil { @@ -752,7 +752,7 @@ func (ac *applicationController) ListPipelines(accounts models.Accounts, w http. // type: string // It was suggested to keep this under /applications/{appName} endpoint, but for now this will be the same for all applications - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) supportedPipelines := handler.GetSupportedPipelines() radixhttp.JSONResponse(w, r, supportedPipelines) } @@ -796,7 +796,7 @@ func (ac *applicationController) TriggerPipelineBuild(accounts models.Accounts, // "404": // description: "Not found" appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) jobSummary, err := handler.TriggerPipelineBuild(r.Context(), appName, r) if err != nil { @@ -847,7 +847,7 @@ func (ac *applicationController) TriggerPipelineBuildDeploy(accounts models.Acco // description: "Not found" appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) jobSummary, err := handler.TriggerPipelineBuildDeploy(r.Context(), appName, r) if err != nil { @@ -898,7 +898,7 @@ func (ac *applicationController) TriggerPipelineDeploy(accounts models.Accounts, // description: "Not found" appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) jobSummary, err := handler.TriggerPipelineDeploy(r.Context(), appName, r) if err != nil { @@ -947,7 +947,7 @@ func (ac *applicationController) TriggerPipelinePromote(accounts models.Accounts // description: "Not found" appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) jobSummary, err := handler.TriggerPipelinePromote(r.Context(), appName, r) if err != nil { diff --git a/api/applications/applications_controller_test.go b/api/applications/applications_controller_test.go index a491ca6e..139ba22b 100644 --- a/api/applications/applications_controller_test.go +++ b/api/applications/applications_controller_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "github.com/equinor/radix-api/models" "net/http" "net/url" "os" @@ -66,8 +67,11 @@ func setupTest(requireAppConfigurationItem, requireAppADGroups bool) (*commontes func(_ context.Context, _ kubernetes.Interface, _ v1.RadixRegistration) (bool, error) { return true, nil }, - NewApplicationHandlerFactory( + newTestApplicationHandlerFactory( ApplicationHandlerConfig{RequireAppConfigurationItem: requireAppConfigurationItem, RequireAppADGroups: requireAppADGroups}, + func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) { + return true, nil + }, ), ), ) @@ -91,7 +95,10 @@ func TestGetApplications_HasAccessToSomeRR(t *testing.T) { NewApplicationController( func(_ context.Context, _ kubernetes.Interface, _ v1.RadixRegistration) (bool, error) { return false, nil - }, NewApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}))) + }, newTestApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}, + func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) { + return true, nil + }))) responseChannel := controllerTestUtils.ExecuteRequest("GET", "/api/v1/applications") response := <-responseChannel @@ -104,7 +111,10 @@ func TestGetApplications_HasAccessToSomeRR(t *testing.T) { controllerTestUtils := controllertest.NewTestUtils(kubeclient, radixclient, secretproviderclient, NewApplicationController( func(_ context.Context, _ kubernetes.Interface, rr v1.RadixRegistration) (bool, error) { return rr.GetName() == "my-second-app", nil - }, NewApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}))) + }, newTestApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}, + func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) { + return true, nil + }))) responseChannel := controllerTestUtils.ExecuteRequest("GET", "/api/v1/applications") response := <-responseChannel @@ -117,7 +127,10 @@ func TestGetApplications_HasAccessToSomeRR(t *testing.T) { controllerTestUtils := controllertest.NewTestUtils(kubeclient, radixclient, secretproviderclient, NewApplicationController( func(_ context.Context, _ kubernetes.Interface, _ v1.RadixRegistration) (bool, error) { return true, nil - }, NewApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}))) + }, newTestApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}, + func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) { + return true, nil + }))) responseChannel := controllerTestUtils.ExecuteRequest("GET", "/api/v1/applications") response := <-responseChannel @@ -185,7 +198,10 @@ func TestSearchApplications(t *testing.T) { controllerTestUtils := controllertest.NewTestUtils(kubeclient, radixclient, secretproviderclient, NewApplicationController( func(_ context.Context, _ kubernetes.Interface, _ v1.RadixRegistration) (bool, error) { return true, nil - }, NewApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}))) + }, newTestApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}, + func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) { + return true, nil + }))) // Tests t.Run("search for "+appNames[0], func(t *testing.T) { @@ -259,7 +275,10 @@ func TestSearchApplications(t *testing.T) { controllerTestUtils := controllertest.NewTestUtils(kubeclient, radixclient, secretproviderclient, NewApplicationController( func(_ context.Context, _ kubernetes.Interface, _ v1.RadixRegistration) (bool, error) { return false, nil - }, NewApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}))) + }, newTestApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}, + func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) { + return true, nil + }))) searchParam := applicationModels.ApplicationsSearchRequest{Names: []string{appNames[0]}} responseChannel := controllerTestUtils.ExecuteRequestWithParameters("POST", "/api/v1/applications/_search", &searchParam) response := <-responseChannel @@ -1606,3 +1625,20 @@ func buildApplicationRegistrationRequest(applicationRegistration applicationMode AcknowledgeWarnings: acknowledgeWarnings, } } + +type testApplicationHandlerFactory struct { + config ApplicationHandlerConfig + hasAccessToGetConfigMap hasAccessToGetConfigMapFunc +} + +func newTestApplicationHandlerFactory(config ApplicationHandlerConfig, hasAccessToGetConfigMap hasAccessToGetConfigMapFunc) ApplicationHandlerFactory { + return &testApplicationHandlerFactory{ + config: config, + hasAccessToGetConfigMap: hasAccessToGetConfigMap, + } +} + +// Create creates a new ApplicationHandler +func (f *testApplicationHandlerFactory) Create(accounts models.Accounts) ApplicationHandler { + return NewApplicationHandler(accounts, f.config, f.hasAccessToGetConfigMap) +} diff --git a/api/applications/applications_handler.go b/api/applications/applications_handler.go index 2f7c0f81..f84a0ea2 100644 --- a/api/applications/applications_handler.go +++ b/api/applications/applications_handler.go @@ -16,7 +16,6 @@ import ( "github.com/equinor/radix-api/api/kubequery" apimodels "github.com/equinor/radix-api/api/models" "github.com/equinor/radix-api/api/utils" - "github.com/equinor/radix-api/api/utils/access" "github.com/equinor/radix-api/models" radixhttp "github.com/equinor/radix-common/net/http" radixutils "github.com/equinor/radix-common/utils" @@ -45,23 +44,27 @@ type patch struct { Value interface{} `json:"value"` } +type hasAccessToGetConfigMapFunc func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) + // ApplicationHandler Instance variables type ApplicationHandler struct { - jobHandler job.JobHandler - environmentHandler environments.EnvironmentHandler - accounts models.Accounts - config ApplicationHandlerConfig - namespace string + jobHandler job.JobHandler + environmentHandler environments.EnvironmentHandler + accounts models.Accounts + config ApplicationHandlerConfig + namespace string + hasAccessToGetConfigMap func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) } // NewApplicationHandler Constructor -func NewApplicationHandler(accounts models.Accounts, config ApplicationHandlerConfig) ApplicationHandler { +func NewApplicationHandler(accounts models.Accounts, config ApplicationHandlerConfig, hasAccessToGetConfigMap hasAccessToGetConfigMapFunc) ApplicationHandler { return ApplicationHandler{ - accounts: accounts, - jobHandler: job.Init(accounts, deployments.Init(accounts)), - environmentHandler: environments.Init(environments.WithAccounts(accounts)), - config: config, - namespace: getApiNamespace(config), + accounts: accounts, + jobHandler: job.Init(accounts, deployments.Init(accounts)), + environmentHandler: environments.Init(environments.WithAccounts(accounts)), + config: config, + namespace: getApiNamespace(config), + hasAccessToGetConfigMap: hasAccessToGetConfigMap, } } @@ -301,6 +304,10 @@ func (ah *ApplicationHandler) ModifyRegistrationDetails(ctx context.Context, app // Only these fields can change over time patchRequest := applicationRegistrationPatchRequest.ApplicationRegistrationPatch if patchRequest.AdGroups != nil && !radixutils.ArrayEqualElements(currentRegistration.Spec.AdGroups, *patchRequest.AdGroups) { + err := ah.validateUserIsMemberOfAdGroups(ctx, appName, updatedRegistration.Spec.AdGroups) + if err != nil { + return nil, err + } updatedRegistration.Spec.AdGroups = *patchRequest.AdGroups payload = append(payload, patch{Op: "replace", Path: "/spec/adGroups", Value: *patchRequest.AdGroups}) runUpdate = true @@ -372,10 +379,6 @@ func (ah *ApplicationHandler) ModifyRegistrationDetails(ctx context.Context, app } if runUpdate { - err := ah.validateUserIsMemberOfAdGroups(ctx, appName, updatedRegistration.Spec.AdGroups) - if err != nil { - return nil, err - } err = ah.isValidRegistrationUpdate(updatedRegistration, currentRegistration) if err != nil { return nil, err @@ -779,14 +782,7 @@ func (ah *ApplicationHandler) validateUserIsMemberOfAdGroups(ctx context.Context } }() - valid, err := access.HasAccess(ctx, ah.accounts.UserAccount.Client, &authorizationapi.ResourceAttributes{ - Verb: "get", - Group: "", - Resource: "configmaps", - Version: "*", - Namespace: ah.namespace, - Name: configMapName, - }) + valid, err := ah.hasAccessToGetConfigMap(ctx, ah.accounts.UserAccount.Client, ah.namespace, configMapName) if err != nil { return err } diff --git a/api/applications/applications_handler_factory.go b/api/applications/applications_handler_factory.go index ab0fddc9..d54a24a9 100644 --- a/api/applications/applications_handler_factory.go +++ b/api/applications/applications_handler_factory.go @@ -1,15 +1,41 @@ package applications import ( + "context" + "github.com/equinor/radix-api/api/utils/access" "github.com/equinor/radix-api/models" + authorizationapi "k8s.io/api/authorization/v1" + "k8s.io/client-go/kubernetes" ) // ApplicationHandlerFactory defines a factory function for creating an ApplicationHandler -type ApplicationHandlerFactory func(accounts models.Accounts) ApplicationHandler +type ApplicationHandlerFactory interface { + Create(accounts models.Accounts) ApplicationHandler +} + +type applicationHandlerFactory struct { + config ApplicationHandlerConfig +} // NewApplicationHandlerFactory creates a new ApplicationHandlerFactory func NewApplicationHandlerFactory(config ApplicationHandlerConfig) ApplicationHandlerFactory { - return func(accounts models.Accounts) ApplicationHandler { - return NewApplicationHandler(accounts, config) + return &applicationHandlerFactory{ + config: config, } } + +// Create creates a new ApplicationHandler +func (f *applicationHandlerFactory) Create(accounts models.Accounts) ApplicationHandler { + return NewApplicationHandler(accounts, f.config, hasAccessToGetConfigMap) +} + +func hasAccessToGetConfigMap(ctx context.Context, kubeClient kubernetes.Interface, namespace, configMapName string) (bool, error) { + return access.HasAccess(ctx, kubeClient, &authorizationapi.ResourceAttributes{ + Verb: "get", + Group: "", + Resource: "configmaps", + Version: "*", + Namespace: namespace, + Name: configMapName, + }) +} From 1fc10f9a5c30c72a3164712f1a7d50d62e557035 Mon Sep 17 00:00:00 2001 From: Sergey Smolnikov Date: Fri, 22 Sep 2023 13:44:04 +0200 Subject: [PATCH 11/12] Fixed unit-tests --- api/applications/applications_handler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/applications/applications_handler.go b/api/applications/applications_handler.go index f84a0ea2..e1d3b756 100644 --- a/api/applications/applications_handler.go +++ b/api/applications/applications_handler.go @@ -304,7 +304,7 @@ func (ah *ApplicationHandler) ModifyRegistrationDetails(ctx context.Context, app // Only these fields can change over time patchRequest := applicationRegistrationPatchRequest.ApplicationRegistrationPatch if patchRequest.AdGroups != nil && !radixutils.ArrayEqualElements(currentRegistration.Spec.AdGroups, *patchRequest.AdGroups) { - err := ah.validateUserIsMemberOfAdGroups(ctx, appName, updatedRegistration.Spec.AdGroups) + err := ah.validateUserIsMemberOfAdGroups(ctx, appName, *patchRequest.AdGroups) if err != nil { return nil, err } From 22fbe8d61ef2989e1715dcfc134c86af7699fb1c Mon Sep 17 00:00:00 2001 From: Sergey Smolnikov Date: Fri, 22 Sep 2023 14:23:45 +0200 Subject: [PATCH 12/12] Added unit-tests --- .../applications_controller_test.go | 117 ++++++++++++++++-- 1 file changed, 110 insertions(+), 7 deletions(-) diff --git a/api/applications/applications_controller_test.go b/api/applications/applications_controller_test.go index 139ba22b..a532b716 100644 --- a/api/applications/applications_controller_test.go +++ b/api/applications/applications_controller_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "github.com/equinor/radix-api/models" + "github.com/equinor/radix-common/utils/pointers" "net/http" "net/url" "os" @@ -47,6 +48,15 @@ const ( ) func setupTest(requireAppConfigurationItem, requireAppADGroups bool) (*commontest.Utils, *controllertest.Utils, *kubefake.Clientset, *fake.Clientset, prometheusclient.Interface, secretsstorevclient.Interface) { + return setupTestWithFactory(newTestApplicationHandlerFactory( + ApplicationHandlerConfig{RequireAppConfigurationItem: requireAppConfigurationItem, RequireAppADGroups: requireAppADGroups}, + func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) { + return true, nil + }, + )) +} + +func setupTestWithFactory(handlerFactory ApplicationHandlerFactory) (*commontest.Utils, *controllertest.Utils, *kubefake.Clientset, *fake.Clientset, prometheusclient.Interface, secretsstorevclient.Interface) { // Setup kubeclient := kubefake.NewSimpleClientset() radixclient := fake.NewSimpleClientset() @@ -67,12 +77,7 @@ func setupTest(requireAppConfigurationItem, requireAppADGroups bool) (*commontes func(_ context.Context, _ kubernetes.Interface, _ v1.RadixRegistration) (bool, error) { return true, nil }, - newTestApplicationHandlerFactory( - ApplicationHandlerConfig{RequireAppConfigurationItem: requireAppConfigurationItem, RequireAppADGroups: requireAppADGroups}, - func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) { - return true, nil - }, - ), + handlerFactory, ), ) @@ -1158,6 +1163,104 @@ func TestModifyApplication_IgnoreRequireADGroupValidationWhenRequiredButCurrentI assert.Equal(t, http.StatusOK, response.Code) } +func TestModifyApplication_UpdateADGroupValidation(t *testing.T) { + type scenario struct { + requireAppADGroups bool + name string + hasAccessToAdGroups bool + expectedResponseCode int + adminAdGroups []string + } + scenarios := []scenario{ + { + name: "Require ADGroups, has groups and has access to them", + requireAppADGroups: true, + adminAdGroups: []string{"e654757d-c789-11e8-bbad-045000000001"}, + hasAccessToAdGroups: true, + expectedResponseCode: http.StatusOK, + }, + { + name: "Require ADGroups, has no groups and has access to them", + requireAppADGroups: true, + adminAdGroups: []string{}, + hasAccessToAdGroups: true, + expectedResponseCode: http.StatusBadRequest, + }, + { + name: "Require ADGroups, has groups and has no access to them", + requireAppADGroups: true, + adminAdGroups: []string{"e654757d-c789-11e8-bbad-045000000001"}, + hasAccessToAdGroups: false, + expectedResponseCode: http.StatusBadRequest, + }, + { + name: "Require ADGroups, has no groups and has no access to them", + requireAppADGroups: true, + adminAdGroups: []string{}, + hasAccessToAdGroups: false, + expectedResponseCode: http.StatusBadRequest, + }, + { + name: "Not require ADGroups, has groups and has access to them", + requireAppADGroups: false, + adminAdGroups: []string{"e654757d-c789-11e8-bbad-045000000001"}, + hasAccessToAdGroups: true, + expectedResponseCode: http.StatusOK, + }, + { + name: "Not require ADGroups, has no groups and has access to them", + requireAppADGroups: false, + adminAdGroups: []string{}, + hasAccessToAdGroups: true, + expectedResponseCode: http.StatusOK, + }, + { + name: "Not require ADGroups, has groups and has no access to them", + requireAppADGroups: false, + adminAdGroups: []string{"e654757d-c789-11e8-bbad-045000000001"}, + hasAccessToAdGroups: false, + expectedResponseCode: http.StatusBadRequest, + }, + { + name: "Not require ADGroups, has no groups and has no access to them", + requireAppADGroups: false, + adminAdGroups: []string{}, + hasAccessToAdGroups: false, + expectedResponseCode: http.StatusOK, + }, + } + + for _, ts := range scenarios { + t.Run(ts.name, func(t *testing.T) { + _, controllerTestUtils, _, radixClient, _, _ := setupTestWithFactory(newTestApplicationHandlerFactory( + ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: ts.requireAppADGroups}, + func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) { + return ts.hasAccessToAdGroups, nil + }, + )) + + rr, err := anApplicationRegistration(). + WithName("any-name"). + WithAdGroups([]string{"e654757d-c789-11e8-bbad-045007777777"}). + BuildRR() + require.NoError(t, err) + _, err = radixClient.RadixV1().RadixRegistrations().Create(context.Background(), rr, metav1.CreateOptions{}) + require.NoError(t, err) + + // Test + patchRequest := applicationModels.ApplicationRegistrationPatchRequest{ + ApplicationRegistrationPatch: &applicationModels.ApplicationRegistrationPatch{ + AdGroups: pointers.Ptr(ts.adminAdGroups), + }, + } + + responseChannel := controllerTestUtils.ExecuteRequestWithParameters("PATCH", fmt.Sprintf("/api/v1/applications/%s", "any-name"), patchRequest) + response := <-responseChannel + assert.Equal(t, ts.expectedResponseCode, response.Code, fmt.Sprintf("Expected response code %d but got %d", ts.expectedResponseCode, response.Code)) + }) + } +} + func TestHandleTriggerPipeline_ForNonMappedAndMappedAndMagicBranchEnvironment_JobIsNotCreatedForUnmapped(t *testing.T) { // Setup commonTestUtils, controllerTestUtils, _, _, _, _ := setupTest(true, true) @@ -1631,7 +1734,7 @@ type testApplicationHandlerFactory struct { hasAccessToGetConfigMap hasAccessToGetConfigMapFunc } -func newTestApplicationHandlerFactory(config ApplicationHandlerConfig, hasAccessToGetConfigMap hasAccessToGetConfigMapFunc) ApplicationHandlerFactory { +func newTestApplicationHandlerFactory(config ApplicationHandlerConfig, hasAccessToGetConfigMap hasAccessToGetConfigMapFunc) *testApplicationHandlerFactory { return &testApplicationHandlerFactory{ config: config, hasAccessToGetConfigMap: hasAccessToGetConfigMap,