Skip to content

Commit

Permalink
Validate user is member of one of existing admin groups (#538)
Browse files Browse the repository at this point in the history 
* Added validation to create a config map

* Validate creating a config map

* Validate get a config map

* Fixed deleting of temp role and rolebinding

* Fixed has access

* Corrected prop name case

* Added validation to create app method. Removed from playground case

* Added warning

* Corrected

* Fixed unit-tests

* Fixed unit-tests

* Added unit-tests
  • Loading branch information
@satr
satr authored Sep 22, 2023
1 parent 28f63f7 commit 93c89b5
Show file tree
Hide file tree
Showing 10 changed files with 379 additions and 66 deletions.
2 changes: 2 additions & 0 deletions .vscode/launch.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
"RADIX_ACTIVE_CLUSTER_EGRESS_IPS": "104.45.84.1",
"REQUIRE_APP_CONFIGURATION_ITEM": "true",
"REQUIRE_APP_AD_GROUPS": "true",
"RADIX_ENVIRONMENT":"qa",
"RADIX_APP":"radix-api"
},
"args": [
"--useOutClusterClient=false"
Expand Down
30 changes: 15 additions & 15 deletions api/applications/applications_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
155 changes: 147 additions & 8 deletions api/applications/applications_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"context"
"encoding/json"
"fmt"
"github.com/equinor/radix-api/models"
"github.com/equinor/radix-common/utils/pointers"
"net/http"
"net/url"
"os"
Expand Down Expand Up @@ -46,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()
Expand All @@ -66,9 +77,7 @@ func setupTest(requireAppConfigurationItem, requireAppADGroups bool) (*commontes
func(_ context.Context, _ kubernetes.Interface, _ v1.RadixRegistration) (bool, error) {
return true, nil
},
NewApplicationHandlerFactory(
ApplicationHandlerConfig{RequireAppConfigurationItem: requireAppConfigurationItem, RequireAppADGroups: requireAppADGroups},
),
handlerFactory,
),
)

Expand All @@ -91,7 +100,10 @@ func TestGetApplications_HasAccessToSomeRR(t *testing.T) {
NewApplicationController(
func(_ context.Context, _ kubernetes.Interface, _ v1.RadixRegistration) (bool, error) {
return false, nil
}, NewApplicationHandlerFactory(ApplicationHandlerConfig{true, 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

Expand All @@ -104,7 +116,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{true, 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

Expand All @@ -117,7 +132,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{true, 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

Expand Down Expand Up @@ -185,7 +203,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{true, 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) {
Expand Down Expand Up @@ -259,7 +280,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{true, 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
Expand Down Expand Up @@ -1139,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)
Expand Down Expand Up @@ -1606,3 +1728,20 @@ func buildApplicationRegistrationRequest(applicationRegistration applicationMode
AcknowledgeWarnings: acknowledgeWarnings,
}
}

type testApplicationHandlerFactory struct {
config ApplicationHandlerConfig
hasAccessToGetConfigMap hasAccessToGetConfigMapFunc
}

func newTestApplicationHandlerFactory(config ApplicationHandlerConfig, hasAccessToGetConfigMap hasAccessToGetConfigMapFunc) *testApplicationHandlerFactory {
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)
}
Loading

0 comments on commit 93c89b5

Please sign in to comment.