From 414cf411ff708159f0fbecfa4acdc3cc40a8fe0e Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 25 Jul 2024 14:10:34 +0200 Subject: [PATCH 1/9] add public-viewer support to pkg/configuration this commit is part of PR #443 Signed-off-by: Francesco Ilario --- pkg/configuration/configuration.go | 4 +++ pkg/configuration/configuration_test.go | 39 +++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/pkg/configuration/configuration.go b/pkg/configuration/configuration.go index a6611d6f..3e49fa68 100644 --- a/pkg/configuration/configuration.go +++ b/pkg/configuration/configuration.go @@ -121,6 +121,10 @@ func (r RegistrationServiceConfig) Print() { logger.Info("Registration Service Configuration", "config", r.cfg.Host.RegistrationService) } +func (r RegistrationServiceConfig) PublicViewerEnabled() bool { + return r.cfg.Host.PublicViewerConfig != nil && r.cfg.Host.PublicViewerConfig.Enabled +} + func (r RegistrationServiceConfig) Environment() string { return commonconfig.GetString(r.cfg.Host.RegistrationService.Environment, prodEnvironment) } diff --git a/pkg/configuration/configuration_test.go b/pkg/configuration/configuration_test.go index 226e0004..812718e9 100644 --- a/pkg/configuration/configuration_test.go +++ b/pkg/configuration/configuration_test.go @@ -3,6 +3,7 @@ package configuration_test import ( "testing" + "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/registration-service/pkg/configuration" "github.com/codeready-toolchain/registration-service/test" commonconfig "github.com/codeready-toolchain/toolchain-common/pkg/configuration" @@ -68,6 +69,7 @@ func TestRegistrationService(t *testing.T) { assert.InDelta(t, float32(0), regServiceCfg.Verification().CaptchaRequiredScore(), 0.01) assert.True(t, regServiceCfg.Verification().CaptchaAllowLowScoreReactivation()) assert.Empty(t, regServiceCfg.Verification().CaptchaServiceAccountFileContents()) + assert.False(t, regServiceCfg.PublicViewerEnabled()) }) t.Run("non-default", func(t *testing.T) { // given @@ -151,5 +153,42 @@ func TestRegistrationService(t *testing.T) { assert.InDelta(t, float32(0.5), regServiceCfg.Verification().CaptchaRequiredScore(), 0.01) assert.False(t, regServiceCfg.Verification().CaptchaAllowLowScoreReactivation()) assert.Equal(t, "example-content", regServiceCfg.Verification().CaptchaServiceAccountFileContents()) + assert.False(t, regServiceCfg.PublicViewerEnabled()) }) } + +func TestPublicViewer(t *testing.T) { + tt := map[string]struct { + name string + expectedValue bool + publicViewerConfig *v1alpha1.PublicViewerConfiguration + }{ + "public-viewer is explicitly enabled": { + expectedValue: true, + publicViewerConfig: &v1alpha1.PublicViewerConfiguration{Enabled: true}, + }, + "public-viewer is explicitly disabled": { + expectedValue: false, + publicViewerConfig: &v1alpha1.PublicViewerConfiguration{Enabled: false}, + }, + "public-viewer config not set, assume disabled": { + expectedValue: false, + publicViewerConfig: nil, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + // given + cfg := commonconfig.NewToolchainConfigObjWithReset(t) + cfg.Spec.Host.PublicViewerConfig = tc.publicViewerConfig + secrets := make(map[string]map[string]string) + + // when + regServiceCfg := configuration.NewRegistrationServiceConfig(cfg, secrets) + + // then + assert.Equal(t, tc.expectedValue, regServiceCfg.PublicViewerEnabled()) + }) + } +} From 06efa5994f6b39c83aad984c6fea883e8ad42c7e Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 25 Jul 2024 14:53:27 +0200 Subject: [PATCH 2/9] add public-viewer support to pkg/context this commit is part of PR #443 Signed-off-by: Francesco Ilario --- pkg/context/keys.go | 4 +++ pkg/context/public_viewer.go | 12 +++++++ pkg/context/public_viewer_test.go | 53 +++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+) create mode 100644 pkg/context/public_viewer.go create mode 100644 pkg/context/public_viewer_test.go diff --git a/pkg/context/keys.go b/pkg/context/keys.go index 9477de21..a6eba535 100644 --- a/pkg/context/keys.go +++ b/pkg/context/keys.go @@ -25,4 +25,8 @@ const ( WorkspaceKey = "workspace" // RequestReceivedTime is the context key for the starting time of a request made RequestReceivedTime = "requestReceivedTime" + // PublicViewerEnabled is a boolean value indicating whether PublicViewer support is enabled + PublicViewerEnabled = "publicViewerEnabled" + // ImpersonateUser is the context key for the impersonated user in proxied call + ImpersonateUser = "impersonateUser" ) diff --git a/pkg/context/public_viewer.go b/pkg/context/public_viewer.go new file mode 100644 index 00000000..d7f11b23 --- /dev/null +++ b/pkg/context/public_viewer.go @@ -0,0 +1,12 @@ +package context + +import ( + "github.com/labstack/echo/v4" +) + +// IsPublicViewerEnabled retrieves from the context the boolean value associated to the PublicViewerEnabled key. +// If the key is not set it returns false, otherwise it returns the boolean value stored in the context. +func IsPublicViewerEnabled(ctx echo.Context) bool { + publicViewerEnabled, _ := ctx.Get(PublicViewerEnabled).(bool) + return publicViewerEnabled +} diff --git a/pkg/context/public_viewer_test.go b/pkg/context/public_viewer_test.go new file mode 100644 index 00000000..9b86d62f --- /dev/null +++ b/pkg/context/public_viewer_test.go @@ -0,0 +1,53 @@ +package context_test + +import ( + "testing" + + "github.com/codeready-toolchain/registration-service/pkg/context" + "github.com/labstack/echo/v4" + "github.com/stretchr/testify/require" +) + +func TestIsPublicViewerEnabled(t *testing.T) { + tt := map[string]struct { + data map[string]interface{} + expected bool + }{ + "context value is true": { + data: map[string]interface{}{context.PublicViewerEnabled: true}, + expected: true, + }, + "context value is false": { + data: map[string]interface{}{context.PublicViewerEnabled: false}, + expected: false, + }, + "value not set in context": { + data: map[string]interface{}{}, + expected: false, + }, + "value set to a not castable value": { + data: map[string]interface{}{context.PublicViewerEnabled: struct{}{}}, + expected: false, + }, + "value set to nil": { + data: map[string]interface{}{context.PublicViewerEnabled: nil}, + expected: false, + }, + } + + for k, tc := range tt { + t.Run(k, func(t *testing.T) { + // given + ctx := echo.New().NewContext(nil, nil) + for k, v := range tc.data { + ctx.Set(k, v) + } + + // when + actual := context.IsPublicViewerEnabled(ctx) + + // then + require.Equal(t, tc.expected, actual, "IsPublicViewerEnabled returned a value different from expected") + }) + } +} From 511ea82eaad8aa8bfc7c3123862b61ce84333966 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 25 Jul 2024 20:22:06 +0200 Subject: [PATCH 3/9] add public-viewer support to SpaceLister's List this commit is part of the effort of splitting PR #443 Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get_test.go | 2 +- pkg/proxy/handlers/spacelister_list.go | 39 ++- pkg/proxy/handlers/spacelister_list_test.go | 304 +++++++++++++------- pkg/proxy/handlers/spacelister_test.go | 26 +- 4 files changed, 247 insertions(+), 124 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index 6195cf92..958b7f8d 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -32,7 +32,7 @@ import ( ) func TestSpaceListerGet(t *testing.T) { - fakeSignupService, fakeClient := buildSpaceListerFakes(t) + fakeSignupService, fakeClient := buildSpaceListerFakes(t, false) memberFakeClient := fake.InitClient(t, // spacebinding requests diff --git a/pkg/proxy/handlers/spacelister_list.go b/pkg/proxy/handlers/spacelister_list.go index 01eb9c90..b0328d0d 100644 --- a/pkg/proxy/handlers/spacelister_list.go +++ b/pkg/proxy/handlers/spacelister_list.go @@ -6,20 +6,24 @@ import ( "net/http" "time" - toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" - "github.com/codeready-toolchain/registration-service/pkg/context" - "github.com/codeready-toolchain/registration-service/pkg/proxy/metrics" "github.com/labstack/echo/v4" errs "github.com/pkg/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/selection" + + toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + "github.com/codeready-toolchain/registration-service/pkg/context" + "github.com/codeready-toolchain/registration-service/pkg/proxy/metrics" + "github.com/codeready-toolchain/registration-service/pkg/signup" ) func HandleSpaceListRequest(spaceLister *SpaceLister) echo.HandlerFunc { return func(ctx echo.Context) error { // list all user workspaces requestReceivedTime := ctx.Get(context.RequestReceivedTime).(time.Time) + ctx.Set(context.PublicViewerEnabled, false) // disable public-viewer on list endpoint workspaces, err := ListUserWorkspaces(ctx, spaceLister) if err != nil { spaceLister.ProxyMetrics.RegServWorkspaceHistogramVec.WithLabelValues(fmt.Sprintf("%d", http.StatusInternalServerError), metrics.MetricsLabelVerbList).Observe(time.Since(requestReceivedTime).Seconds()) // using list as the default value for verb to minimize label combinations for prometheus to process @@ -41,17 +45,33 @@ func ListUserWorkspaces(ctx echo.Context, spaceLister *SpaceLister) ([]toolchain if signup == nil { return []toolchainv1alpha1.Workspace{}, nil } - murName := signup.CompliantUsername + + // get MUR Names + murNames := getMURNamesForList(ctx, signup) // get all spacebindings with given mur since no workspace was provided - spaceBindings, err := listSpaceBindingsForUser(spaceLister, murName) + spaceBindings, err := listSpaceBindingsForUsers(spaceLister, murNames) if err != nil { ctx.Logger().Error(errs.Wrap(err, "error listing space bindings")) return nil, err } + return workspacesFromSpaceBindings(ctx, spaceLister, signup.Name, spaceBindings), nil } +// getMURNamesForList returns a list of MasterUserRecord names to use for listing Workspaces. +// If PublicViewer is enabled, the list will also contain the PublicViewer username. +func getMURNamesForList(ctx echo.Context, signup *signup.Signup) []string { + names := []string{} + if signup != nil && signup.CompliantUsername != "" { + names = append(names, signup.CompliantUsername) + } + if context.IsPublicViewerEnabled(ctx) { + names = append(names, toolchainv1alpha1.KubesawAuthenticatedUsername) + } + return names +} + func listWorkspaceResponse(ctx echo.Context, workspaces []toolchainv1alpha1.Workspace) error { workspaceList := &toolchainv1alpha1.WorkspaceList{ TypeMeta: metav1.TypeMeta{ @@ -66,9 +86,12 @@ func listWorkspaceResponse(ctx echo.Context, workspaces []toolchainv1alpha1.Work return json.NewEncoder(ctx.Response().Writer).Encode(workspaceList) } -func listSpaceBindingsForUser(spaceLister *SpaceLister, murName string) ([]toolchainv1alpha1.SpaceBinding, error) { - mrr, _ := labels.SelectorFromSet(labels.Set{toolchainv1alpha1.SpaceBindingMasterUserRecordLabelKey: murName}).Requirements() - return spaceLister.GetInformerServiceFunc().ListSpaceBindings(mrr[0]) +func listSpaceBindingsForUsers(spaceLister *SpaceLister, murNames []string) ([]toolchainv1alpha1.SpaceBinding, error) { + murSelector, err := labels.NewRequirement(toolchainv1alpha1.SpaceBindingMasterUserRecordLabelKey, selection.In, murNames) + if err != nil { + return nil, err + } + return spaceLister.GetInformerServiceFunc().ListSpaceBindings(*murSelector) } func workspacesFromSpaceBindings(ctx echo.Context, spaceLister *SpaceLister, signupName string, spaceBindings []toolchainv1alpha1.SpaceBinding) []toolchainv1alpha1.Workspace { diff --git a/pkg/proxy/handlers/spacelister_list_test.go b/pkg/proxy/handlers/spacelister_list_test.go index c07b4a09..5937cb5e 100644 --- a/pkg/proxy/handlers/spacelister_list_test.go +++ b/pkg/proxy/handlers/spacelister_list_test.go @@ -22,127 +22,211 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/proxy/metrics" "github.com/codeready-toolchain/registration-service/pkg/signup" "github.com/codeready-toolchain/registration-service/test/fake" + "github.com/codeready-toolchain/toolchain-common/pkg/test" ) -func TestSpaceListerList(t *testing.T) { - fakeSignupService, fakeClient := buildSpaceListerFakes(t) - - t.Run("HandleSpaceListRequest", func(t *testing.T) { - // given - tests := map[string]struct { - username string - expectedWs []toolchainv1alpha1.Workspace - expectedErr string - expectedErrCode int - expectedWorkspace string - overrideSignupFunc func(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error) - overrideInformerFunc func() service.InformerService - }{ - "dancelover lists spaces": { - username: "dance.lover", - expectedWs: []toolchainv1alpha1.Workspace{ +func TestSpaceListerListUserWorkspaces(t *testing.T) { + tests := map[string]struct { + username string + expectedWs func(*test.FakeClient) []toolchainv1alpha1.Workspace + expectedErr string + expectedWorkspace string + publicViewerEnabled bool + }{ + "dancelover lists spaces with public-viewer enabled": { + username: "dance.lover", + expectedWs: func(fakeClient *test.FakeClient) []toolchainv1alpha1.Workspace { + return []toolchainv1alpha1.Workspace{ + workspaceFor(t, fakeClient, "communityspace", "viewer", false), workspaceFor(t, fakeClient, "dancelover", "admin", true), workspaceFor(t, fakeClient, "movielover", "other", false), - }, - expectedErr: "", - }, - "movielover lists spaces": { - username: "movie.lover", - expectedWs: []toolchainv1alpha1.Workspace{ - workspaceFor(t, fakeClient, "movielover", "admin", true), - }, - expectedErr: "", - }, - "signup has no compliant username set": { - username: "racing.lover", - expectedWs: []toolchainv1alpha1.Workspace{}, - expectedErr: "", - expectedErrCode: 200, - }, - "space not initialized yet": { - username: "panda.lover", - expectedWs: []toolchainv1alpha1.Workspace{}, - expectedErr: "", - expectedErrCode: 200, + } }, - "no spaces found": { - username: "user.nospace", - expectedWs: []toolchainv1alpha1.Workspace{}, - expectedErr: "", - expectedErrCode: 200, + expectedErr: "", + publicViewerEnabled: true, + }, + "dancelover lists spaces with public-viewer disabled": { + username: "dance.lover", + expectedWs: func(fakeClient *test.FakeClient) []toolchainv1alpha1.Workspace { + return []toolchainv1alpha1.Workspace{ + workspaceFor(t, fakeClient, "dancelover", "admin", true), + workspaceFor(t, fakeClient, "movielover", "other", false), + } }, - "informer error": { - username: "dance.lover", - expectedWs: []toolchainv1alpha1.Workspace{}, - expectedErr: "list spacebindings error", - expectedErrCode: 500, - overrideInformerFunc: func() service.InformerService { - inf := fake.NewFakeInformer() - inf.ListSpaceBindingFunc = func(_ ...labels.Requirement) ([]toolchainv1alpha1.SpaceBinding, error) { - return nil, fmt.Errorf("list spacebindings error") - } - return inf + expectedErr: "", + publicViewerEnabled: false, + }, + } + + for k, tc := range tests { + fakeSignupService, fakeClient := buildSpaceListerFakes(t, tc.publicViewerEnabled) + + t.Run(k, func(t *testing.T) { + // given + signupProvider := fakeSignupService.GetSignupFromInformer + informerFunc := fake.GetInformerService(fakeClient) + + proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) + + s := &handlers.SpaceLister{ + GetSignupFunc: signupProvider, + GetInformerServiceFunc: informerFunc, + ProxyMetrics: proxyMetrics, + } + + e := echo.New() + req := httptest.NewRequest(http.MethodGet, "/", strings.NewReader("")) + rec := httptest.NewRecorder() + ctx := e.NewContext(req, rec) + ctx.Set(rcontext.UsernameKey, tc.username) + ctx.Set(rcontext.RequestReceivedTime, time.Now()) + + // when + ctx.Set(rcontext.PublicViewerEnabled, tc.publicViewerEnabled) + ww, err := handlers.ListUserWorkspaces(ctx, s) + + // then + require.NoError(t, err) + // list workspace case + expectedWs := tc.expectedWs(fakeClient) + require.Equal(t, len(expectedWs), len(ww)) + for i, w := range ww { + assert.Equal(t, expectedWs[i].Name, w.Name) + assert.Equal(t, expectedWs[i].Status, w.Status) + } + }) + } +} + +func TestSpaceListerList(t *testing.T) { + tt := map[string]struct { + publicViewerEnabled bool + }{ + "public-viewer is enabled": {publicViewerEnabled: true}, + "public-viewer is disabled": {publicViewerEnabled: false}, + } + + for k, rtc := range tt { + fakeSignupService, fakeClient := buildSpaceListerFakes(t, rtc.publicViewerEnabled) + + t.Run(k, func(t *testing.T) { + // given + tests := map[string]struct { + username string + expectedWs []toolchainv1alpha1.Workspace + expectedErr string + expectedErrCode int + expectedWorkspace string + overrideSignupFunc func(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error) + overrideInformerFunc func() service.InformerService + }{ + "dancelover lists spaces": { + username: "dance.lover", + expectedWs: []toolchainv1alpha1.Workspace{ + workspaceFor(t, fakeClient, "dancelover", "admin", true), + workspaceFor(t, fakeClient, "movielover", "other", false), + }, + expectedErr: "", }, - }, - "get signup error": { - username: "dance.lover", - expectedWs: []toolchainv1alpha1.Workspace{}, - expectedErr: "signup error", - expectedErrCode: 500, - overrideSignupFunc: func(_ *gin.Context, _, _ string, _ bool) (*signup.Signup, error) { - return nil, fmt.Errorf("signup error") + "movielover lists spaces": { + username: "movie.lover", + expectedWs: []toolchainv1alpha1.Workspace{ + workspaceFor(t, fakeClient, "movielover", "admin", true), + }, + expectedErr: "", }, - }, - } - - for k, tc := range tests { - t.Run(k, func(t *testing.T) { - // given - signupProvider := fakeSignupService.GetSignupFromInformer - if tc.overrideSignupFunc != nil { - signupProvider = tc.overrideSignupFunc - } + "signup has no compliant username set": { + username: "racing.lover", + expectedWs: []toolchainv1alpha1.Workspace{}, + expectedErr: "", + expectedErrCode: 200, + }, + "space not initialized yet": { + username: "panda.lover", + expectedWs: []toolchainv1alpha1.Workspace{}, + expectedErr: "", + expectedErrCode: 200, + }, + "no spaces found": { + username: "user.nospace", + expectedWs: []toolchainv1alpha1.Workspace{}, + expectedErr: "", + expectedErrCode: 200, + }, + "informer error": { + username: "dance.lover", + expectedWs: []toolchainv1alpha1.Workspace{}, + expectedErr: "list spacebindings error", + expectedErrCode: 500, + overrideInformerFunc: func() service.InformerService { + inf := fake.NewFakeInformer() + inf.ListSpaceBindingFunc = func(_ ...labels.Requirement) ([]toolchainv1alpha1.SpaceBinding, error) { + return nil, fmt.Errorf("list spacebindings error") + } + return inf + }, + }, + "get signup error": { + username: "dance.lover", + expectedWs: []toolchainv1alpha1.Workspace{}, + expectedErr: "signup error", + expectedErrCode: 500, + overrideSignupFunc: func(_ *gin.Context, _, _ string, _ bool) (*signup.Signup, error) { + return nil, fmt.Errorf("signup error") + }, + }, + } - informerFunc := fake.GetInformerService(fakeClient) - if tc.overrideInformerFunc != nil { - informerFunc = tc.overrideInformerFunc - } + for k, tc := range tests { + t.Run(k, func(t *testing.T) { + // given + signupProvider := fakeSignupService.GetSignupFromInformer + if tc.overrideSignupFunc != nil { + signupProvider = tc.overrideSignupFunc + } - proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) + informerFunc := fake.GetInformerService(fakeClient) + if tc.overrideInformerFunc != nil { + informerFunc = tc.overrideInformerFunc + } - s := &handlers.SpaceLister{ - GetSignupFunc: signupProvider, - GetInformerServiceFunc: informerFunc, - ProxyMetrics: proxyMetrics, - } + proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) - e := echo.New() - req := httptest.NewRequest(http.MethodGet, "/", strings.NewReader("")) - rec := httptest.NewRecorder() - ctx := e.NewContext(req, rec) - ctx.Set(rcontext.UsernameKey, tc.username) - ctx.Set(rcontext.RequestReceivedTime, time.Now()) - - // when - err := handlers.HandleSpaceListRequest(s)(ctx) - - // then - if tc.expectedErr != "" { - // error case - require.Equal(t, tc.expectedErrCode, rec.Code) - require.Contains(t, rec.Body.String(), tc.expectedErr) - } else { - require.NoError(t, err) - // list workspace case - workspaceList, decodeErr := decodeResponseToWorkspaceList(rec.Body.Bytes()) - require.NoError(t, decodeErr) - require.Equal(t, len(tc.expectedWs), len(workspaceList.Items)) - for i := range tc.expectedWs { - assert.Equal(t, tc.expectedWs[i].Name, workspaceList.Items[i].Name) - assert.Equal(t, tc.expectedWs[i].Status, workspaceList.Items[i].Status) + s := &handlers.SpaceLister{ + GetSignupFunc: signupProvider, + GetInformerServiceFunc: informerFunc, + ProxyMetrics: proxyMetrics, } - } - }) - } - }) + + e := echo.New() + req := httptest.NewRequest(http.MethodGet, "/", strings.NewReader("")) + rec := httptest.NewRecorder() + ctx := e.NewContext(req, rec) + ctx.Set(rcontext.UsernameKey, tc.username) + ctx.Set(rcontext.RequestReceivedTime, time.Now()) + + // when + ctx.Set(rcontext.PublicViewerEnabled, rtc.publicViewerEnabled) + err := handlers.HandleSpaceListRequest(s)(ctx) + + // then + if tc.expectedErr != "" { + // error case + require.Equal(t, tc.expectedErrCode, rec.Code) + require.Contains(t, rec.Body.String(), tc.expectedErr) + } else { + require.NoError(t, err) + // list workspace case + workspaceList, decodeErr := decodeResponseToWorkspaceList(rec.Body.Bytes()) + require.NoError(t, decodeErr) + require.Equal(t, len(tc.expectedWs), len(workspaceList.Items)) + for i := range tc.expectedWs { + assert.Equal(t, tc.expectedWs[i].Name, workspaceList.Items[i].Name) + assert.Equal(t, tc.expectedWs[i].Status, workspaceList.Items[i].Status) + } + } + }) + } + }) + } } diff --git a/pkg/proxy/handlers/spacelister_test.go b/pkg/proxy/handlers/spacelister_test.go index f9804e74..82f2854f 100644 --- a/pkg/proxy/handlers/spacelister_test.go +++ b/pkg/proxy/handlers/spacelister_test.go @@ -20,8 +20,8 @@ import ( spacetest "github.com/codeready-toolchain/toolchain-common/pkg/test/space" ) -func buildSpaceListerFakes(t *testing.T) (*fake.SignupService, *test.FakeClient) { - fakeSignupService := fake.NewSignupService( +func buildSpaceListerFakes(t *testing.T, publicViewerEnabled bool) (*fake.SignupService, *test.FakeClient) { + signups := []fake.SignupDef{ newSignup("dancelover", "dance.lover", true), newSignup("movielover", "movie.lover", true), newSignup("pandalover", "panda.lover", true), @@ -33,7 +33,14 @@ func buildSpaceListerFakes(t *testing.T) (*fake.SignupService, *test.FakeClient) newSignup("parentspace", "parent.space", true), newSignup("childspace", "child.space", true), newSignup("grandchildspace", "grandchild.space", true), - ) + } + if publicViewerEnabled { + signups = append(signups, + newSignup("communityspace", "community.space", true), + newSignup("communitylover", "community.lover", true), + ) + } + fakeSignupService := fake.NewSignupService(signups...) // space that is not provisioned yet spaceNotProvisionedYet := fake.NewSpace("pandalover", "member-2", "pandalover") @@ -60,7 +67,7 @@ func buildSpaceListerFakes(t *testing.T) (*fake.SignupService, *test.FakeClient) spaceBindingWithInvalidSBRNamespace.Labels[toolchainv1alpha1.SpaceBindingRequestLabelKey] = "anime-sbr" spaceBindingWithInvalidSBRNamespace.Labels[toolchainv1alpha1.SpaceBindingRequestNamespaceLabelKey] = "" // let's set the name to blank in order to trigger an error - fakeClient := fake.InitClient(t, + objs := []runtime.Object{ // spaces fake.NewSpace("dancelover", "member-1", "dancelover"), fake.NewSpace("movielover", "member-1", "movielover"), @@ -74,6 +81,8 @@ func buildSpaceListerFakes(t *testing.T) (*fake.SignupService, *test.FakeClient) fake.NewSpace("grandchildspace", "member-1", "grandchildspace", spacetest.WithSpecParentSpace("childspace")), // noise space, user will have a different role here , just to make sure this is not returned anywhere in the tests fake.NewSpace("otherspace", "member-1", "otherspace", spacetest.WithSpecParentSpace("otherspace")), + // space flagged as community + fake.NewSpace("communityspace", "member-2", "communityspace"), //spacebindings fake.NewSpaceBinding("dancer-sb1", "dancelover", "dancelover", "admin"), @@ -94,7 +103,14 @@ func buildSpaceListerFakes(t *testing.T) (*fake.SignupService, *test.FakeClient) //nstemplatetier fake.NewBase1NSTemplateTier(), - ) + } + if publicViewerEnabled { + objs = append(objs, + fake.NewSpaceBinding("communityspace-sb", "communityspace", "communityspace", "admin"), + fake.NewSpaceBinding("community-sb", toolchainv1alpha1.KubesawAuthenticatedUsername, "communityspace", "viewer"), + ) + } + fakeClient := fake.InitClient(t, objs...) return fakeSignupService, fakeClient } From 9327aafbac84aa39979ea90aea974f8954485941 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 1 Aug 2024 11:53:58 +0200 Subject: [PATCH 4/9] cleanup tests Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_list_test.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_list_test.go b/pkg/proxy/handlers/spacelister_list_test.go index 5937cb5e..47de9917 100644 --- a/pkg/proxy/handlers/spacelister_list_test.go +++ b/pkg/proxy/handlers/spacelister_list_test.go @@ -28,32 +28,28 @@ import ( func TestSpaceListerListUserWorkspaces(t *testing.T) { tests := map[string]struct { username string - expectedWs func(*test.FakeClient) []toolchainv1alpha1.Workspace - expectedErr string - expectedWorkspace string + expectedWorkspaces func(*test.FakeClient) []toolchainv1alpha1.Workspace publicViewerEnabled bool }{ "dancelover lists spaces with public-viewer enabled": { username: "dance.lover", - expectedWs: func(fakeClient *test.FakeClient) []toolchainv1alpha1.Workspace { + expectedWorkspaces: func(fakeClient *test.FakeClient) []toolchainv1alpha1.Workspace { return []toolchainv1alpha1.Workspace{ workspaceFor(t, fakeClient, "communityspace", "viewer", false), workspaceFor(t, fakeClient, "dancelover", "admin", true), workspaceFor(t, fakeClient, "movielover", "other", false), } }, - expectedErr: "", publicViewerEnabled: true, }, "dancelover lists spaces with public-viewer disabled": { username: "dance.lover", - expectedWs: func(fakeClient *test.FakeClient) []toolchainv1alpha1.Workspace { + expectedWorkspaces: func(fakeClient *test.FakeClient) []toolchainv1alpha1.Workspace { return []toolchainv1alpha1.Workspace{ workspaceFor(t, fakeClient, "dancelover", "admin", true), workspaceFor(t, fakeClient, "movielover", "other", false), } }, - expectedErr: "", publicViewerEnabled: false, }, } @@ -88,7 +84,7 @@ func TestSpaceListerListUserWorkspaces(t *testing.T) { // then require.NoError(t, err) // list workspace case - expectedWs := tc.expectedWs(fakeClient) + expectedWs := tc.expectedWorkspaces(fakeClient) require.Equal(t, len(expectedWs), len(ww)) for i, w := range ww { assert.Equal(t, expectedWs[i].Name, w.Name) From 256f14fba965643232ec6572991857c6e8657efc Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Wed, 7 Aug 2024 16:43:02 +0200 Subject: [PATCH 5/9] Update pkg/proxy/handlers/spacelister_list_test.go --- pkg/proxy/handlers/spacelister_list_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/handlers/spacelister_list_test.go b/pkg/proxy/handlers/spacelister_list_test.go index 47de9917..91b30912 100644 --- a/pkg/proxy/handlers/spacelister_list_test.go +++ b/pkg/proxy/handlers/spacelister_list_test.go @@ -25,7 +25,7 @@ import ( "github.com/codeready-toolchain/toolchain-common/pkg/test" ) -func TestSpaceListerListUserWorkspaces(t *testing.T) { +func TestListUserWorkspaces(t *testing.T) { tests := map[string]struct { username string expectedWorkspaces func(*test.FakeClient) []toolchainv1alpha1.Workspace From a0cc9fd14bbb9af12011649875b02b4898169cc6 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 13 Aug 2024 11:00:34 +0200 Subject: [PATCH 6/9] improve tests Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get_test.go | 2 +- pkg/proxy/handlers/spacelister_list_test.go | 18 ++++++++++--- pkg/proxy/handlers/spacelister_test.go | 30 +++++++++------------ 3 files changed, 29 insertions(+), 21 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index 958b7f8d..6195cf92 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -32,7 +32,7 @@ import ( ) func TestSpaceListerGet(t *testing.T) { - fakeSignupService, fakeClient := buildSpaceListerFakes(t, false) + fakeSignupService, fakeClient := buildSpaceListerFakes(t) memberFakeClient := fake.InitClient(t, // spacebinding requests diff --git a/pkg/proxy/handlers/spacelister_list_test.go b/pkg/proxy/handlers/spacelister_list_test.go index 91b30912..7016832d 100644 --- a/pkg/proxy/handlers/spacelister_list_test.go +++ b/pkg/proxy/handlers/spacelister_list_test.go @@ -14,6 +14,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/labels" + "sigs.k8s.io/controller-runtime/pkg/client" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/registration-service/pkg/application/service" @@ -22,20 +23,31 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/proxy/metrics" "github.com/codeready-toolchain/registration-service/pkg/signup" "github.com/codeready-toolchain/registration-service/test/fake" + "github.com/codeready-toolchain/toolchain-common/pkg/proxy" "github.com/codeready-toolchain/toolchain-common/pkg/test" + "github.com/codeready-toolchain/toolchain-common/pkg/test/space" ) func TestListUserWorkspaces(t *testing.T) { tests := map[string]struct { username string + additionalSignups []fake.SignupDef + additionalObjects []client.Object expectedWorkspaces func(*test.FakeClient) []toolchainv1alpha1.Workspace publicViewerEnabled bool }{ "dancelover lists spaces with public-viewer enabled": { username: "dance.lover", + additionalSignups: []fake.SignupDef{ + newSignup("communitylover", "community.lover", true), + }, + additionalObjects: []client.Object{ + fake.NewSpace("communityspace", "member-1", "communitylover", space.WithTierName("appstudio")), + fake.NewSpaceBinding("communitylover", toolchainv1alpha1.KubesawAuthenticatedUsername, "communityspace", "viewer"), + }, expectedWorkspaces: func(fakeClient *test.FakeClient) []toolchainv1alpha1.Workspace { return []toolchainv1alpha1.Workspace{ - workspaceFor(t, fakeClient, "communityspace", "viewer", false), + workspaceFor(t, fakeClient, "communityspace", "viewer", false, proxy.WithOwner("communitylover")), workspaceFor(t, fakeClient, "dancelover", "admin", true), workspaceFor(t, fakeClient, "movielover", "other", false), } @@ -55,7 +67,7 @@ func TestListUserWorkspaces(t *testing.T) { } for k, tc := range tests { - fakeSignupService, fakeClient := buildSpaceListerFakes(t, tc.publicViewerEnabled) + fakeSignupService, fakeClient := buildSpaceListerFakesWithResources(t, tc.additionalSignups, tc.additionalObjects) t.Run(k, func(t *testing.T) { // given @@ -103,7 +115,7 @@ func TestSpaceListerList(t *testing.T) { } for k, rtc := range tt { - fakeSignupService, fakeClient := buildSpaceListerFakes(t, rtc.publicViewerEnabled) + fakeSignupService, fakeClient := buildSpaceListerFakes(t) t.Run(k, func(t *testing.T) { // given diff --git a/pkg/proxy/handlers/spacelister_test.go b/pkg/proxy/handlers/spacelister_test.go index 82f2854f..7e08efc1 100644 --- a/pkg/proxy/handlers/spacelister_test.go +++ b/pkg/proxy/handlers/spacelister_test.go @@ -20,8 +20,12 @@ import ( spacetest "github.com/codeready-toolchain/toolchain-common/pkg/test/space" ) -func buildSpaceListerFakes(t *testing.T, publicViewerEnabled bool) (*fake.SignupService, *test.FakeClient) { - signups := []fake.SignupDef{ +func buildSpaceListerFakes(t *testing.T) (*fake.SignupService, *test.FakeClient) { + return buildSpaceListerFakesWithResources(t, nil, nil) +} + +func buildSpaceListerFakesWithResources(t *testing.T, signups []fake.SignupDef, objs []client.Object) (*fake.SignupService, *test.FakeClient) { + ss := []fake.SignupDef{ newSignup("dancelover", "dance.lover", true), newSignup("movielover", "movie.lover", true), newSignup("pandalover", "panda.lover", true), @@ -34,13 +38,10 @@ func buildSpaceListerFakes(t *testing.T, publicViewerEnabled bool) (*fake.Signup newSignup("childspace", "child.space", true), newSignup("grandchildspace", "grandchild.space", true), } - if publicViewerEnabled { - signups = append(signups, - newSignup("communityspace", "community.space", true), - newSignup("communitylover", "community.lover", true), - ) + for _, s := range signups { + ss = append(ss, s) } - fakeSignupService := fake.NewSignupService(signups...) + fakeSignupService := fake.NewSignupService(ss...) // space that is not provisioned yet spaceNotProvisionedYet := fake.NewSpace("pandalover", "member-2", "pandalover") @@ -67,7 +68,7 @@ func buildSpaceListerFakes(t *testing.T, publicViewerEnabled bool) (*fake.Signup spaceBindingWithInvalidSBRNamespace.Labels[toolchainv1alpha1.SpaceBindingRequestLabelKey] = "anime-sbr" spaceBindingWithInvalidSBRNamespace.Labels[toolchainv1alpha1.SpaceBindingRequestNamespaceLabelKey] = "" // let's set the name to blank in order to trigger an error - objs := []runtime.Object{ + oo := []runtime.Object{ // spaces fake.NewSpace("dancelover", "member-1", "dancelover"), fake.NewSpace("movielover", "member-1", "movielover"), @@ -81,8 +82,6 @@ func buildSpaceListerFakes(t *testing.T, publicViewerEnabled bool) (*fake.Signup fake.NewSpace("grandchildspace", "member-1", "grandchildspace", spacetest.WithSpecParentSpace("childspace")), // noise space, user will have a different role here , just to make sure this is not returned anywhere in the tests fake.NewSpace("otherspace", "member-1", "otherspace", spacetest.WithSpecParentSpace("otherspace")), - // space flagged as community - fake.NewSpace("communityspace", "member-2", "communityspace"), //spacebindings fake.NewSpaceBinding("dancer-sb1", "dancelover", "dancelover", "admin"), @@ -104,13 +103,10 @@ func buildSpaceListerFakes(t *testing.T, publicViewerEnabled bool) (*fake.Signup //nstemplatetier fake.NewBase1NSTemplateTier(), } - if publicViewerEnabled { - objs = append(objs, - fake.NewSpaceBinding("communityspace-sb", "communityspace", "communityspace", "admin"), - fake.NewSpaceBinding("community-sb", toolchainv1alpha1.KubesawAuthenticatedUsername, "communityspace", "viewer"), - ) + for _, o := range objs { + oo = append(oo, o) } - fakeClient := fake.InitClient(t, objs...) + fakeClient := fake.InitClient(t, oo...) return fakeSignupService, fakeClient } From 2f018f851de0b6e0cf9bac9c456207b4e5932cf1 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 13 Aug 2024 11:02:27 +0200 Subject: [PATCH 7/9] rename test func Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_list_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/handlers/spacelister_list_test.go b/pkg/proxy/handlers/spacelister_list_test.go index 7016832d..4c9fada5 100644 --- a/pkg/proxy/handlers/spacelister_list_test.go +++ b/pkg/proxy/handlers/spacelister_list_test.go @@ -106,7 +106,7 @@ func TestListUserWorkspaces(t *testing.T) { } } -func TestSpaceListerList(t *testing.T) { +func TestHandleSpaceListRequest(t *testing.T) { tt := map[string]struct { publicViewerEnabled bool }{ From ca493e7c3e0fa1ecb8f2f2b4d52905219d3750d5 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 13 Aug 2024 11:07:24 +0200 Subject: [PATCH 8/9] fix linter complaints Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_list_test.go | 6 +++--- pkg/proxy/handlers/spacelister_test.go | 16 +++++----------- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_list_test.go b/pkg/proxy/handlers/spacelister_list_test.go index 4c9fada5..cc9d0266 100644 --- a/pkg/proxy/handlers/spacelister_list_test.go +++ b/pkg/proxy/handlers/spacelister_list_test.go @@ -14,7 +14,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/labels" - "sigs.k8s.io/controller-runtime/pkg/client" + "k8s.io/apimachinery/pkg/runtime" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/registration-service/pkg/application/service" @@ -32,7 +32,7 @@ func TestListUserWorkspaces(t *testing.T) { tests := map[string]struct { username string additionalSignups []fake.SignupDef - additionalObjects []client.Object + additionalObjects []runtime.Object expectedWorkspaces func(*test.FakeClient) []toolchainv1alpha1.Workspace publicViewerEnabled bool }{ @@ -41,7 +41,7 @@ func TestListUserWorkspaces(t *testing.T) { additionalSignups: []fake.SignupDef{ newSignup("communitylover", "community.lover", true), }, - additionalObjects: []client.Object{ + additionalObjects: []runtime.Object{ fake.NewSpace("communityspace", "member-1", "communitylover", space.WithTierName("appstudio")), fake.NewSpaceBinding("communitylover", toolchainv1alpha1.KubesawAuthenticatedUsername, "communityspace", "viewer"), }, diff --git a/pkg/proxy/handlers/spacelister_test.go b/pkg/proxy/handlers/spacelister_test.go index 7e08efc1..f03b465d 100644 --- a/pkg/proxy/handlers/spacelister_test.go +++ b/pkg/proxy/handlers/spacelister_test.go @@ -24,8 +24,8 @@ func buildSpaceListerFakes(t *testing.T) (*fake.SignupService, *test.FakeClient) return buildSpaceListerFakesWithResources(t, nil, nil) } -func buildSpaceListerFakesWithResources(t *testing.T, signups []fake.SignupDef, objs []client.Object) (*fake.SignupService, *test.FakeClient) { - ss := []fake.SignupDef{ +func buildSpaceListerFakesWithResources(t *testing.T, signups []fake.SignupDef, objs []runtime.Object) (*fake.SignupService, *test.FakeClient) { + ss := append(signups, newSignup("dancelover", "dance.lover", true), newSignup("movielover", "movie.lover", true), newSignup("pandalover", "panda.lover", true), @@ -37,10 +37,7 @@ func buildSpaceListerFakesWithResources(t *testing.T, signups []fake.SignupDef, newSignup("parentspace", "parent.space", true), newSignup("childspace", "child.space", true), newSignup("grandchildspace", "grandchild.space", true), - } - for _, s := range signups { - ss = append(ss, s) - } + ) fakeSignupService := fake.NewSignupService(ss...) // space that is not provisioned yet @@ -68,7 +65,7 @@ func buildSpaceListerFakesWithResources(t *testing.T, signups []fake.SignupDef, spaceBindingWithInvalidSBRNamespace.Labels[toolchainv1alpha1.SpaceBindingRequestLabelKey] = "anime-sbr" spaceBindingWithInvalidSBRNamespace.Labels[toolchainv1alpha1.SpaceBindingRequestNamespaceLabelKey] = "" // let's set the name to blank in order to trigger an error - oo := []runtime.Object{ + oo := append(objs, // spaces fake.NewSpace("dancelover", "member-1", "dancelover"), fake.NewSpace("movielover", "member-1", "movielover"), @@ -102,10 +99,7 @@ func buildSpaceListerFakesWithResources(t *testing.T, signups []fake.SignupDef, //nstemplatetier fake.NewBase1NSTemplateTier(), - } - for _, o := range objs { - oo = append(oo, o) - } + ) fakeClient := fake.InitClient(t, oo...) return fakeSignupService, fakeClient From c913d90d3fde77676e706662c65f521576294805 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 13 Aug 2024 11:37:21 +0200 Subject: [PATCH 9/9] refactor Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_list_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_list_test.go b/pkg/proxy/handlers/spacelister_list_test.go index cc9d0266..1402f367 100644 --- a/pkg/proxy/handlers/spacelister_list_test.go +++ b/pkg/proxy/handlers/spacelister_list_test.go @@ -23,7 +23,6 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/proxy/metrics" "github.com/codeready-toolchain/registration-service/pkg/signup" "github.com/codeready-toolchain/registration-service/test/fake" - "github.com/codeready-toolchain/toolchain-common/pkg/proxy" "github.com/codeready-toolchain/toolchain-common/pkg/test" "github.com/codeready-toolchain/toolchain-common/pkg/test/space" ) @@ -42,12 +41,12 @@ func TestListUserWorkspaces(t *testing.T) { newSignup("communitylover", "community.lover", true), }, additionalObjects: []runtime.Object{ - fake.NewSpace("communityspace", "member-1", "communitylover", space.WithTierName("appstudio")), - fake.NewSpaceBinding("communitylover", toolchainv1alpha1.KubesawAuthenticatedUsername, "communityspace", "viewer"), + fake.NewSpace("communitylover", "member-1", "communitylover", space.WithTierName("appstudio")), + fake.NewSpaceBinding("communitylover-publicviewer", toolchainv1alpha1.KubesawAuthenticatedUsername, "communitylover", "viewer"), }, expectedWorkspaces: func(fakeClient *test.FakeClient) []toolchainv1alpha1.Workspace { return []toolchainv1alpha1.Workspace{ - workspaceFor(t, fakeClient, "communityspace", "viewer", false, proxy.WithOwner("communitylover")), + workspaceFor(t, fakeClient, "communitylover", "viewer", false), workspaceFor(t, fakeClient, "dancelover", "admin", true), workspaceFor(t, fakeClient, "movielover", "other", false), }