Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Validate user is member of one of existing admin groups #538

Merged
merged 12 commits into from
Sep 22, 2023
Merged

Validate user is member of one of existing admin groups #538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
93c2d88
Added validation to create a config map
satr Sep 14, 2023
cce371a
Validate creating a config map
satr Sep 15, 2023
5b2417d
Validate get a config map
satr Sep 20, 2023
5b2c4e5
Fixed deleting of temp role and rolebinding
satr Sep 20, 2023
d69c5bf
Fixed has access
satr Sep 21, 2023
1a2a567
Corrected prop name case
satr Sep 21, 2023
216b455
Added validation to create app method. Removed from playground case
satr Sep 22, 2023
8cccf45
Added warning
satr Sep 22, 2023
579dded
Corrected
satr Sep 22, 2023
edd057e
Fixed unit-tests
satr Sep 22, 2023
1fc10f9
Fixed unit-tests
satr Sep 22, 2023
22fbe8d
Added unit-tests
satr Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .vscode/launch.json
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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