From 414cf411ff708159f0fbecfa4acdc3cc40a8fe0e Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 25 Jul 2024 14:10:34 +0200 Subject: [PATCH 01/19] 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 02/19] 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 99e41e8f95871c89adb33dccd82284aef9d82718 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 25 Jul 2024 20:14:19 +0200 Subject: [PATCH 03/19] add public-viewer support to SpaceLister's Get this commit is part of the effort of splitting PR #443 Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get.go | 84 ++++++- pkg/proxy/handlers/spacelister_get_test.go | 249 +++++++++++++++++++- pkg/proxy/handlers/spacelister_list_test.go | 2 +- pkg/proxy/handlers/spacelister_test.go | 26 +- 4 files changed, 344 insertions(+), 17 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get.go b/pkg/proxy/handlers/spacelister_get.go index 0572f063..7033c60d 100644 --- a/pkg/proxy/handlers/spacelister_get.go +++ b/pkg/proxy/handlers/spacelister_get.go @@ -8,6 +8,8 @@ import ( "time" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + "github.com/codeready-toolchain/registration-service/pkg/configuration" + "github.com/codeready-toolchain/registration-service/pkg/context" regsercontext "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" @@ -26,6 +28,8 @@ func HandleSpaceGetRequest(spaceLister *SpaceLister, GetMembersFunc cluster.GetM // get specific workspace return func(ctx echo.Context) error { requestReceivedTime := ctx.Get(regsercontext.RequestReceivedTime).(time.Time) + publicViewerEnabled := configuration.GetRegistrationServiceConfig().PublicViewerEnabled() + ctx.Set(context.PublicViewerEnabled, publicViewerEnabled) workspace, err := GetUserWorkspaceWithBindings(ctx, spaceLister, ctx.Param("workspace"), GetMembersFunc) if err != nil { spaceLister.ProxyMetrics.RegServWorkspaceHistogramVec.WithLabelValues(fmt.Sprintf("%d", http.StatusInternalServerError), metrics.MetricsLabelVerbGet).Observe(time.Since(requestReceivedTime).Seconds()) // using list as the default value for verb to minimize label combinations for prometheus to process @@ -42,7 +46,7 @@ func HandleSpaceGetRequest(spaceLister *SpaceLister, GetMembersFunc cluster.GetM } } -// GetUserWorkspace returns a workspace object with the required fields used by the proxy +// GetUserWorkspace returns a workspace object with the required fields used by the proxy. func GetUserWorkspace(ctx echo.Context, spaceLister *SpaceLister, workspaceName string) (*toolchainv1alpha1.Workspace, error) { userSignup, space, err := getUserSignupAndSpace(ctx, spaceLister, workspaceName) if err != nil { @@ -53,10 +57,58 @@ func GetUserWorkspace(ctx echo.Context, spaceLister *SpaceLister, workspaceName return nil, nil } + // retrieve user space binding + userSpaceBinding, err := getUserOrPublicViewerSpaceBinding(ctx, spaceLister, space, userSignup, workspaceName) + if err != nil { + return nil, err + } + // consider this as not found + if userSpaceBinding == nil { + return nil, nil + } + + // create and return the result workspace object + return createWorkspaceObject(userSignup.Name, space, userSpaceBinding), nil +} + +// getUserOrPublicViewerSpaceBinding retrieves the user space binding for an user and a space. +// If the SpaceBinding is not found and the PublicViewer feature is enabled, it will retry +// with the PublicViewer credentials. +func getUserOrPublicViewerSpaceBinding(ctx echo.Context, spaceLister *SpaceLister, space *toolchainv1alpha1.Space, userSignup *signup.Signup, workspaceName string) (*toolchainv1alpha1.SpaceBinding, error) { + userSpaceBinding, err := getUserSpaceBinding(ctx, spaceLister, space, userSignup.CompliantUsername) + if err != nil { + return nil, err + } + + // if user space binding is not found and PublicViewer is enabled, + // retry with PublicViewer's signup + if userSpaceBinding == nil { + if context.IsPublicViewerEnabled(ctx) { + pvSb, err := getUserSpaceBinding(ctx, spaceLister, space, toolchainv1alpha1.KubesawAuthenticatedUsername) + if err != nil { + ctx.Logger().Error(fmt.Sprintf("error checking if SpaceBinding is present for user %s and the workspace %s", toolchainv1alpha1.KubesawAuthenticatedUsername, workspaceName)) + return nil, err + } + if pvSb == nil { + ctx.Logger().Error(fmt.Sprintf("unauthorized access - there is no SpaceBinding present for the user %s and the workspace %s", toolchainv1alpha1.KubesawAuthenticatedUsername, workspaceName)) + return nil, nil + } + return pvSb, nil + } + ctx.Logger().Error(fmt.Sprintf("unauthorized access - there is no SpaceBinding present for the user %s and the workspace %s", userSignup.CompliantUsername, workspaceName)) + } + + return userSpaceBinding, nil +} + +// getUserSpaceBinding retrieves the user space binding for an user and a space. +// If no space binding found for this user and space then returns nil, nil +// If multiple found then returns the first one. +func getUserSpaceBinding(ctx echo.Context, spaceLister *SpaceLister, space *toolchainv1alpha1.Space, compliantUsername string) (*toolchainv1alpha1.SpaceBinding, error) { // recursively get all the spacebindings for the current workspace listSpaceBindingsFunc := func(spaceName string) ([]toolchainv1alpha1.SpaceBinding, error) { spaceSelector, _ := labels.SelectorFromSet(labels.Set{toolchainv1alpha1.SpaceBindingSpaceLabelKey: spaceName}).Requirements() - murSelector, _ := labels.SelectorFromSet(labels.Set{toolchainv1alpha1.SpaceBindingMasterUserRecordLabelKey: userSignup.CompliantUsername}).Requirements() + murSelector, _ := labels.SelectorFromSet(labels.Set{toolchainv1alpha1.SpaceBindingMasterUserRecordLabelKey: compliantUsername}).Requirements() return spaceLister.GetInformerServiceFunc().ListSpaceBindings(spaceSelector[0], murSelector[0]) } spaceBindingLister := spacebinding.NewLister(listSpaceBindingsFunc, spaceLister.GetInformerServiceFunc().GetSpace) @@ -66,21 +118,20 @@ func GetUserWorkspace(ctx echo.Context, spaceLister *SpaceLister, workspaceName return nil, err } if len(userSpaceBindings) == 0 { - // let's only log the issue and consider this as not found - ctx.Logger().Error(fmt.Sprintf("unauthorized access - there is no SpaceBinding present for the user %s and the workspace %s", userSignup.CompliantUsername, workspaceName)) + // consider this as not found return nil, nil } if len(userSpaceBindings) > 1 { - userBindingsErr := fmt.Errorf("invalid number of SpaceBindings found for MUR:%s and Space:%s. Expected 1 got %d", userSignup.CompliantUsername, space.Name, len(userSpaceBindings)) + userBindingsErr := fmt.Errorf("invalid number of SpaceBindings found for MUR:%s and Space:%s. Expected 1 got %d", compliantUsername, space.Name, len(userSpaceBindings)) ctx.Logger().Error(userBindingsErr) return nil, userBindingsErr } - return createWorkspaceObject(userSignup.Name, space, &userSpaceBindings[0]), nil + return &userSpaceBindings[0], nil } -// GetUserWorkspaceWithBindings returns a workspace object with the required fields+bindings (the list with all the users access details) +// GetUserWorkspaceWithBindings returns a workspace object with the required fields+bindings (the list with all the users access details). func GetUserWorkspaceWithBindings(ctx echo.Context, spaceLister *SpaceLister, workspaceName string, GetMembersFunc cluster.GetMemberClustersFunc) (*toolchainv1alpha1.Workspace, error) { userSignup, space, err := getUserSignupAndSpace(ctx, spaceLister, workspaceName) if err != nil { @@ -106,9 +157,16 @@ func GetUserWorkspaceWithBindings(ctx echo.Context, spaceLister *SpaceLister, wo // check if user has access to this workspace userBinding := filterUserSpaceBinding(userSignup.CompliantUsername, allSpaceBindings) if userBinding == nil { - // let's only log the issue and consider this as not found - ctx.Logger().Error(fmt.Sprintf("unauthorized access - there is no SpaceBinding present for the user %s and the workspace %s", userSignup.CompliantUsername, workspaceName)) - return nil, nil + // if PublicViewer is enabled, check if the Space is visibile to PublicViewer + if context.IsPublicViewerEnabled(ctx) && userSignup.CompliantUsername != toolchainv1alpha1.KubesawAuthenticatedUsername { + userBinding = filterUserSpaceBinding(toolchainv1alpha1.KubesawAuthenticatedUsername, allSpaceBindings) + } + + if userBinding == nil { + // let's only log the issue and consider this as not found + ctx.Logger().Error(fmt.Sprintf("unauthorized access - there is no SpaceBinding present for the user %s and the workspace %s", userSignup.CompliantUsername, workspaceName)) + return nil, nil + } } // list all SpaceBindingRequests , just in case there might be some failing to create a SpaceBinding resource. @@ -145,6 +203,12 @@ func getUserSignupAndSpace(ctx echo.Context, spaceLister *SpaceLister, workspace if err != nil { return nil, nil, err } + if userSignup == nil && context.IsPublicViewerEnabled(ctx) { + userSignup = &signup.Signup{ + CompliantUsername: toolchainv1alpha1.KubesawAuthenticatedUsername, + Name: toolchainv1alpha1.KubesawAuthenticatedUsername, + } + } space, err := spaceLister.GetInformerServiceFunc().GetSpace(workspaceName) if err != nil { diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index 6195cf92..086809a5 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -32,7 +32,22 @@ import ( ) func TestSpaceListerGet(t *testing.T) { - fakeSignupService, fakeClient := buildSpaceListerFakes(t) + tt := map[string]struct { + publicViewerEnabled bool + }{ + "public-viewer enabled": {publicViewerEnabled: true}, + "public-viewer disabled": {publicViewerEnabled: false}, + } + + for k, tc := range tt { + t.Run(k, func(t *testing.T) { + testSpaceListerGet(t, tc.publicViewerEnabled) + }) + } +} + +func testSpaceListerGet(t *testing.T, publicViewerEnabled bool) { + fakeSignupService, fakeClient := buildSpaceListerFakes(t, publicViewerEnabled) memberFakeClient := fake.InitClient(t, // spacebinding requests @@ -564,6 +579,7 @@ func TestSpaceListerGet(t *testing.T) { if tc.overrideGetMembersFunc != nil { getMembersFunc = tc.overrideGetMembersFunc } + ctx.Set(rcontext.PublicViewerEnabled, publicViewerEnabled) err := handlers.HandleSpaceGetRequest(s, getMembersFunc)(ctx) // then @@ -726,3 +742,234 @@ func TestGetUserWorkspace(t *testing.T) { }) } } + +func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { + + fakeSignupService := fake.NewSignupService( + newSignup("batman", "batman.space", true), + newSignup("robin", "robin.space", true), + ) + + fakeClient := fake.InitClient(t, + // space + fake.NewSpace("robin", "member-1", "robin"), + fake.NewSpace("batman", "member-1", "batman"), + + // spacebindings + fake.NewSpaceBinding("robin-1", "robin", "robin", "admin"), + fake.NewSpaceBinding("batman-1", "batman", "batman", "admin"), + + fake.NewSpaceBinding("community-robin", toolchainv1alpha1.KubesawAuthenticatedUsername, "robin", "viewer"), + ) + + robinWS := workspaceFor(t, fakeClient, "robin", "admin", true) + batmanWS := workspaceFor(t, fakeClient, "batman", "admin", true) + publicRobinWS := func() *toolchainv1alpha1.Workspace { + batmansRobinWS := robinWS.DeepCopy() + batmansRobinWS.Status.Type = "" + batmansRobinWS.Status.Role = "viewer" + return batmansRobinWS + }() + tests := map[string]struct { + username string + expectedErr string + workspaceRequest string + expectedWorkspace *toolchainv1alpha1.Workspace + }{ + "robin can get robin workspace": { + username: "robin.space", + workspaceRequest: "robin", + expectedWorkspace: &robinWS, + }, + "batman can get batman workspace": { + username: "batman.space", + workspaceRequest: "batman", + expectedWorkspace: &batmanWS, + }, + "batman can get robin workspace as public-viewer": { + username: "batman.space", + workspaceRequest: "robin", + expectedWorkspace: publicRobinWS, + }, + "robin can not get batman workspace": { + username: "robin.space", + workspaceRequest: "batman", + expectedWorkspace: nil, + expectedErr: "", + }, + "gordon can get batman workspace": { + username: "gordon.space", + workspaceRequest: "robin", + expectedWorkspace: publicRobinWS, + }, + } + + for k, tc := range tests { + + 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()) + ctx.SetParamNames("workspace") + ctx.SetParamValues(tc.workspaceRequest) + ctx.Set(rcontext.PublicViewerEnabled, true) + + // when + wrk, err := handlers.GetUserWorkspace(ctx, s, tc.workspaceRequest) + + // then + if tc.expectedErr != "" { + // error case + require.Error(t, err, tc.expectedErr) + } else { + require.NoError(t, err) + } + + if tc.expectedWorkspace != nil { + require.Equal(t, tc.expectedWorkspace, wrk) + } else { + require.Nil(t, wrk) // user is not authorized to get this workspace + } + }) + } +} + +func TestSpaceListerGetWithBindingsWithPublicViewerEnabled(t *testing.T) { + + fakeSignupService := fake.NewSignupService( + newSignup("batman", "batman.space", true), + newSignup("robin", "robin.space", true), + ) + + fakeClient := fake.InitClient(t, + // NSTemplateTiers + fake.NewBase1NSTemplateTier(), + + // space + fake.NewSpace("robin", "member-1", "robin"), + fake.NewSpace("batman", "member-1", "batman"), + + // spacebindings + fake.NewSpaceBinding("robin-1", "robin", "robin", "admin"), + fake.NewSpaceBinding("batman-1", "batman", "batman", "admin"), + + fake.NewSpaceBinding("community-robin", toolchainv1alpha1.KubesawAuthenticatedUsername, "robin", "viewer"), + ) + + robinWS := workspaceFor(t, fakeClient, "robin", "admin", true, + commonproxy.WithBindings([]toolchainv1alpha1.Binding{ + { + MasterUserRecord: toolchainv1alpha1.KubesawAuthenticatedUsername, + Role: "viewer", + AvailableActions: []string{}, + }, + { + MasterUserRecord: "robin", + Role: "admin", + AvailableActions: []string{}, + }, + }), + commonproxy.WithAvailableRoles([]string{"admin", "viewer"}), + ) + + batmanWS := workspaceFor(t, fakeClient, "batman", "admin", true, + commonproxy.WithBindings([]toolchainv1alpha1.Binding{ + { + MasterUserRecord: "batman", + Role: "admin", + AvailableActions: []string{}, + }, + }), + commonproxy.WithAvailableRoles([]string{"admin", "viewer"}), + ) + + tests := map[string]struct { + username string + workspaceRequest string + expectedWorkspace *toolchainv1alpha1.Workspace + }{ + "robin can get robin workspace": { + username: "robin.space", + workspaceRequest: "robin", + expectedWorkspace: &robinWS, + }, + "batman can get batman workspace": { + username: "batman.space", + workspaceRequest: "batman", + expectedWorkspace: &batmanWS, + }, + "batman can get robin workspace": { + username: "batman.space", + workspaceRequest: "robin", + expectedWorkspace: func() *toolchainv1alpha1.Workspace { + batmansRobinWS := robinWS.DeepCopy() + batmansRobinWS.Status.Type = "" + batmansRobinWS.Status.Role = "viewer" + return batmansRobinWS + }(), + }, + "robin can not get batman workspace": { + username: "robin.space", + workspaceRequest: "batman", + expectedWorkspace: nil, + }, + } + + for k, tc := range tests { + + 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()) + ctx.SetParamNames("workspace") + ctx.SetParamValues(tc.workspaceRequest) + ctx.Set(rcontext.PublicViewerEnabled, true) + + getMembersFuncMock := func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { + return []*commoncluster.CachedToolchainCluster{ + { + Client: fake.InitClient(t), + Config: &commoncluster.Config{ + Name: "not-me", + }, + }, + } + } + + // when + wrk, err := handlers.GetUserWorkspaceWithBindings(ctx, s, tc.workspaceRequest, getMembersFuncMock) + + // then + require.NoError(t, err) + require.Equal(t, tc.expectedWorkspace, wrk) + }) + } +} diff --git a/pkg/proxy/handlers/spacelister_list_test.go b/pkg/proxy/handlers/spacelister_list_test.go index c07b4a09..0fb9c760 100644 --- a/pkg/proxy/handlers/spacelister_list_test.go +++ b/pkg/proxy/handlers/spacelister_list_test.go @@ -25,7 +25,7 @@ import ( ) func TestSpaceListerList(t *testing.T) { - fakeSignupService, fakeClient := buildSpaceListerFakes(t) + fakeSignupService, fakeClient := buildSpaceListerFakes(t, false) t.Run("HandleSpaceListRequest", func(t *testing.T) { // given 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 5412a9142db49a507c281b49e82d7c788749147d Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 1 Aug 2024 11:05:47 +0200 Subject: [PATCH 04/19] simplify logs by using Errorf Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get.go b/pkg/proxy/handlers/spacelister_get.go index 7033c60d..467dcfe0 100644 --- a/pkg/proxy/handlers/spacelister_get.go +++ b/pkg/proxy/handlers/spacelister_get.go @@ -86,16 +86,16 @@ func getUserOrPublicViewerSpaceBinding(ctx echo.Context, spaceLister *SpaceListe if context.IsPublicViewerEnabled(ctx) { pvSb, err := getUserSpaceBinding(ctx, spaceLister, space, toolchainv1alpha1.KubesawAuthenticatedUsername) if err != nil { - ctx.Logger().Error(fmt.Sprintf("error checking if SpaceBinding is present for user %s and the workspace %s", toolchainv1alpha1.KubesawAuthenticatedUsername, workspaceName)) + ctx.Logger().Errorf("error checking if SpaceBinding is present for user %s and the workspace %s", toolchainv1alpha1.KubesawAuthenticatedUsername, workspaceName) return nil, err } if pvSb == nil { - ctx.Logger().Error(fmt.Sprintf("unauthorized access - there is no SpaceBinding present for the user %s and the workspace %s", toolchainv1alpha1.KubesawAuthenticatedUsername, workspaceName)) + ctx.Logger().Errorf("unauthorized access - there is no SpaceBinding present for the user %s and the workspace %s", toolchainv1alpha1.KubesawAuthenticatedUsername, workspaceName) return nil, nil } return pvSb, nil } - ctx.Logger().Error(fmt.Sprintf("unauthorized access - there is no SpaceBinding present for the user %s and the workspace %s", userSignup.CompliantUsername, workspaceName)) + ctx.Logger().Errorf("unauthorized access - there is no SpaceBinding present for the user %s and the workspace %s", userSignup.CompliantUsername, workspaceName) } return userSpaceBinding, nil @@ -164,7 +164,7 @@ func GetUserWorkspaceWithBindings(ctx echo.Context, spaceLister *SpaceLister, wo if userBinding == nil { // let's only log the issue and consider this as not found - ctx.Logger().Error(fmt.Sprintf("unauthorized access - there is no SpaceBinding present for the user %s and the workspace %s", userSignup.CompliantUsername, workspaceName)) + ctx.Logger().Errorf("unauthorized access - there is no SpaceBinding present for the user %s and the workspace %s", userSignup.CompliantUsername, workspaceName) return nil, nil } } From d1b21d0d4474abef22eb89e9db3410c1130df8b9 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 1 Aug 2024 11:08:31 +0200 Subject: [PATCH 05/19] improve logs Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get.go b/pkg/proxy/handlers/spacelister_get.go index 467dcfe0..18ed1b8b 100644 --- a/pkg/proxy/handlers/spacelister_get.go +++ b/pkg/proxy/handlers/spacelister_get.go @@ -77,6 +77,7 @@ func GetUserWorkspace(ctx echo.Context, spaceLister *SpaceLister, workspaceName func getUserOrPublicViewerSpaceBinding(ctx echo.Context, spaceLister *SpaceLister, space *toolchainv1alpha1.Space, userSignup *signup.Signup, workspaceName string) (*toolchainv1alpha1.SpaceBinding, error) { userSpaceBinding, err := getUserSpaceBinding(ctx, spaceLister, space, userSignup.CompliantUsername) if err != nil { + ctx.Logger().Errorf("error checking if SpaceBinding is present for user %s and the workspace %s: %v", toolchainv1alpha1.KubesawAuthenticatedUsername, workspaceName, err) return nil, err } @@ -86,7 +87,7 @@ func getUserOrPublicViewerSpaceBinding(ctx echo.Context, spaceLister *SpaceListe if context.IsPublicViewerEnabled(ctx) { pvSb, err := getUserSpaceBinding(ctx, spaceLister, space, toolchainv1alpha1.KubesawAuthenticatedUsername) if err != nil { - ctx.Logger().Errorf("error checking if SpaceBinding is present for user %s and the workspace %s", toolchainv1alpha1.KubesawAuthenticatedUsername, workspaceName) + ctx.Logger().Errorf("error checking if SpaceBinding is present for user %s and the workspace %s: %v", toolchainv1alpha1.KubesawAuthenticatedUsername, workspaceName, err) return nil, err } if pvSb == nil { @@ -114,7 +115,6 @@ func getUserSpaceBinding(ctx echo.Context, spaceLister *SpaceLister, space *tool spaceBindingLister := spacebinding.NewLister(listSpaceBindingsFunc, spaceLister.GetInformerServiceFunc().GetSpace) userSpaceBindings, err := spaceBindingLister.ListForSpace(space, []toolchainv1alpha1.SpaceBinding{}) if err != nil { - ctx.Logger().Error(err, "failed to list space bindings") return nil, err } if len(userSpaceBindings) == 0 { @@ -124,7 +124,6 @@ func getUserSpaceBinding(ctx echo.Context, spaceLister *SpaceLister, space *tool if len(userSpaceBindings) > 1 { userBindingsErr := fmt.Errorf("invalid number of SpaceBindings found for MUR:%s and Space:%s. Expected 1 got %d", compliantUsername, space.Name, len(userSpaceBindings)) - ctx.Logger().Error(userBindingsErr) return nil, userBindingsErr } From c34dfad3d890e827b4e7cab8be9a67426c4e1fb4 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 1 Aug 2024 11:11:42 +0200 Subject: [PATCH 06/19] fix logs Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/handlers/spacelister_get.go b/pkg/proxy/handlers/spacelister_get.go index 18ed1b8b..284b29d0 100644 --- a/pkg/proxy/handlers/spacelister_get.go +++ b/pkg/proxy/handlers/spacelister_get.go @@ -91,7 +91,7 @@ func getUserOrPublicViewerSpaceBinding(ctx echo.Context, spaceLister *SpaceListe return nil, err } if pvSb == nil { - ctx.Logger().Errorf("unauthorized access - there is no SpaceBinding present for the user %s and the workspace %s", toolchainv1alpha1.KubesawAuthenticatedUsername, workspaceName) + ctx.Logger().Errorf("unauthorized access - there is no SpaceBinding present for the user %s or %s and the workspace %s", userSignup.CompliantUsername, toolchainv1alpha1.KubesawAuthenticatedUsername, workspaceName) return nil, nil } return pvSb, nil From 0af2c8fe7b29d4cf3b7254c764c290eacc6c1764 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 1 Aug 2024 11:13:49 +0200 Subject: [PATCH 07/19] remove unused parameter from getUserSpaceBinding Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get.go b/pkg/proxy/handlers/spacelister_get.go index 284b29d0..c8fe866f 100644 --- a/pkg/proxy/handlers/spacelister_get.go +++ b/pkg/proxy/handlers/spacelister_get.go @@ -75,7 +75,7 @@ func GetUserWorkspace(ctx echo.Context, spaceLister *SpaceLister, workspaceName // If the SpaceBinding is not found and the PublicViewer feature is enabled, it will retry // with the PublicViewer credentials. func getUserOrPublicViewerSpaceBinding(ctx echo.Context, spaceLister *SpaceLister, space *toolchainv1alpha1.Space, userSignup *signup.Signup, workspaceName string) (*toolchainv1alpha1.SpaceBinding, error) { - userSpaceBinding, err := getUserSpaceBinding(ctx, spaceLister, space, userSignup.CompliantUsername) + userSpaceBinding, err := getUserSpaceBinding(spaceLister, space, userSignup.CompliantUsername) if err != nil { ctx.Logger().Errorf("error checking if SpaceBinding is present for user %s and the workspace %s: %v", toolchainv1alpha1.KubesawAuthenticatedUsername, workspaceName, err) return nil, err @@ -85,7 +85,7 @@ func getUserOrPublicViewerSpaceBinding(ctx echo.Context, spaceLister *SpaceListe // retry with PublicViewer's signup if userSpaceBinding == nil { if context.IsPublicViewerEnabled(ctx) { - pvSb, err := getUserSpaceBinding(ctx, spaceLister, space, toolchainv1alpha1.KubesawAuthenticatedUsername) + pvSb, err := getUserSpaceBinding(spaceLister, space, toolchainv1alpha1.KubesawAuthenticatedUsername) if err != nil { ctx.Logger().Errorf("error checking if SpaceBinding is present for user %s and the workspace %s: %v", toolchainv1alpha1.KubesawAuthenticatedUsername, workspaceName, err) return nil, err @@ -105,7 +105,7 @@ func getUserOrPublicViewerSpaceBinding(ctx echo.Context, spaceLister *SpaceListe // getUserSpaceBinding retrieves the user space binding for an user and a space. // If no space binding found for this user and space then returns nil, nil // If multiple found then returns the first one. -func getUserSpaceBinding(ctx echo.Context, spaceLister *SpaceLister, space *toolchainv1alpha1.Space, compliantUsername string) (*toolchainv1alpha1.SpaceBinding, error) { +func getUserSpaceBinding(spaceLister *SpaceLister, space *toolchainv1alpha1.Space, compliantUsername string) (*toolchainv1alpha1.SpaceBinding, error) { // recursively get all the spacebindings for the current workspace listSpaceBindingsFunc := func(spaceName string) ([]toolchainv1alpha1.SpaceBinding, error) { spaceSelector, _ := labels.SelectorFromSet(labels.Set{toolchainv1alpha1.SpaceBindingSpaceLabelKey: spaceName}).Requirements() From 2d452c91321891a433a0720a7986268df9700dbf Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 1 Aug 2024 11:17:19 +0200 Subject: [PATCH 08/19] update comments Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get.go b/pkg/proxy/handlers/spacelister_get.go index c8fe866f..1fb54d7d 100644 --- a/pkg/proxy/handlers/spacelister_get.go +++ b/pkg/proxy/handlers/spacelister_get.go @@ -102,9 +102,9 @@ func getUserOrPublicViewerSpaceBinding(ctx echo.Context, spaceLister *SpaceListe return userSpaceBinding, nil } -// getUserSpaceBinding retrieves the user space binding for an user and a space. -// If no space binding found for this user and space then returns nil, nil -// If multiple found then returns the first one. +// getUserSpaceBinding retrieves the space binding for an user and a space. +// If no space binding is found for the user and space then it returns nil, nil. +// If more than one space bindings are found, then it returns an error. func getUserSpaceBinding(spaceLister *SpaceLister, space *toolchainv1alpha1.Space, compliantUsername string) (*toolchainv1alpha1.SpaceBinding, error) { // recursively get all the spacebindings for the current workspace listSpaceBindingsFunc := func(spaceName string) ([]toolchainv1alpha1.SpaceBinding, error) { From 9f455b3d03b96c6488bf51bdc701d1b93b87e237 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 1 Aug 2024 11:26:42 +0200 Subject: [PATCH 09/19] remove context's PublicViewerEnabled set Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get.go b/pkg/proxy/handlers/spacelister_get.go index 1fb54d7d..ae4409c4 100644 --- a/pkg/proxy/handlers/spacelister_get.go +++ b/pkg/proxy/handlers/spacelister_get.go @@ -8,7 +8,6 @@ import ( "time" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" - "github.com/codeready-toolchain/registration-service/pkg/configuration" "github.com/codeready-toolchain/registration-service/pkg/context" regsercontext "github.com/codeready-toolchain/registration-service/pkg/context" "github.com/codeready-toolchain/registration-service/pkg/proxy/metrics" @@ -28,8 +27,6 @@ func HandleSpaceGetRequest(spaceLister *SpaceLister, GetMembersFunc cluster.GetM // get specific workspace return func(ctx echo.Context) error { requestReceivedTime := ctx.Get(regsercontext.RequestReceivedTime).(time.Time) - publicViewerEnabled := configuration.GetRegistrationServiceConfig().PublicViewerEnabled() - ctx.Set(context.PublicViewerEnabled, publicViewerEnabled) workspace, err := GetUserWorkspaceWithBindings(ctx, spaceLister, ctx.Param("workspace"), GetMembersFunc) if err != nil { spaceLister.ProxyMetrics.RegServWorkspaceHistogramVec.WithLabelValues(fmt.Sprintf("%d", http.StatusInternalServerError), metrics.MetricsLabelVerbGet).Observe(time.Since(requestReceivedTime).Seconds()) // using list as the default value for verb to minimize label combinations for prometheus to process From af7bb99eb78a40f3e966581e5038f715019d7f26 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 5 Aug 2024 17:35:46 +0200 Subject: [PATCH 10/19] remove expectedErr Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get_test.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index 086809a5..85017b93 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -772,7 +772,6 @@ func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { }() tests := map[string]struct { username string - expectedErr string workspaceRequest string expectedWorkspace *toolchainv1alpha1.Workspace }{ @@ -795,7 +794,6 @@ func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { username: "robin.space", workspaceRequest: "batman", expectedWorkspace: nil, - expectedErr: "", }, "gordon can get batman workspace": { username: "gordon.space", @@ -832,12 +830,7 @@ func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { wrk, err := handlers.GetUserWorkspace(ctx, s, tc.workspaceRequest) // then - if tc.expectedErr != "" { - // error case - require.Error(t, err, tc.expectedErr) - } else { - require.NoError(t, err) - } + require.NoError(t, err) if tc.expectedWorkspace != nil { require.Equal(t, tc.expectedWorkspace, wrk) From 873612414d8de8307a1a4df96e2749a96673e7f8 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 5 Aug 2024 17:40:03 +0200 Subject: [PATCH 11/19] fix test case title Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index 85017b93..129ed36f 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -795,7 +795,7 @@ func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { workspaceRequest: "batman", expectedWorkspace: nil, }, - "gordon can get batman workspace": { + "gordon can get robin workspace": { username: "gordon.space", workspaceRequest: "robin", expectedWorkspace: publicRobinWS, From a525db583e260bde5f677a42e6c1ebe3906c1e12 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 5 Aug 2024 17:51:21 +0200 Subject: [PATCH 12/19] add missing tests Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index 129ed36f..d5ad7814 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -619,6 +619,7 @@ func TestGetUserWorkspace(t *testing.T) { // 2 spacebindings to force the error fake.NewSpaceBinding("batman-1", "batman", "batman", "admin"), fake.NewSpaceBinding("batman-2", "batman", "batman", "maintainer"), + fake.NewSpaceBinding("community-robin", toolchainv1alpha1.KubesawAuthenticatedUsername, "robin", "viewer"), ) robinWS := workspaceFor(t, fakeClient, "robin", "admin", true) @@ -693,6 +694,18 @@ func TestGetUserWorkspace(t *testing.T) { }, expectedWorkspace: nil, }, + "kubesaw-authenticated can not get robin workspace": { + username: toolchainv1alpha1.KubesawAuthenticatedUsername, + workspaceRequest: "robin", + expectedErr: "", + expectedWorkspace: nil, + }, + "batman can not get robin workspace": { + username: "batman.space", + workspaceRequest: "robin", + expectedErr: "", + expectedWorkspace: nil, + }, } for k, tc := range tests { From 7b3c4cd8c640ae16cfb497bda66271fbf3812279 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 5 Aug 2024 19:03:43 +0200 Subject: [PATCH 13/19] Update pkg/proxy/handlers/spacelister_get_test.go Co-authored-by: Alexey Kazakov --- pkg/proxy/handlers/spacelister_get_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index d5ad7814..f70400b8 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -694,7 +694,7 @@ func TestGetUserWorkspace(t *testing.T) { }, expectedWorkspace: nil, }, - "kubesaw-authenticated can not get robin workspace": { + "kubesaw-authenticated can not get robin workspace": { // Because public viewer feature is NOT enabled username: toolchainv1alpha1.KubesawAuthenticatedUsername, workspaceRequest: "robin", expectedErr: "", From 9d94638cec198b3512dace0a54c2b8bbc6a55181 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 5 Aug 2024 19:04:02 +0200 Subject: [PATCH 14/19] Update pkg/proxy/handlers/spacelister_get_test.go Co-authored-by: Alexey Kazakov --- pkg/proxy/handlers/spacelister_get_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index f70400b8..40f17e83 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -700,7 +700,7 @@ func TestGetUserWorkspace(t *testing.T) { expectedErr: "", expectedWorkspace: nil, }, - "batman can not get robin workspace": { + "batman can not get robin workspace": { // Because public viewer feature is NOT enabled username: "batman.space", workspaceRequest: "robin", expectedErr: "", From ab258fd76edbb5473484db744b237132c2b1bf06 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 6 Aug 2024 13:07:26 +0200 Subject: [PATCH 15/19] Update pkg/proxy/handlers/spacelister_get.go Co-authored-by: Francisc Munteanu --- pkg/proxy/handlers/spacelister_get.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/proxy/handlers/spacelister_get.go b/pkg/proxy/handlers/spacelister_get.go index ae4409c4..576ac5c9 100644 --- a/pkg/proxy/handlers/spacelister_get.go +++ b/pkg/proxy/handlers/spacelister_get.go @@ -154,6 +154,7 @@ func GetUserWorkspaceWithBindings(ctx echo.Context, spaceLister *SpaceLister, wo userBinding := filterUserSpaceBinding(userSignup.CompliantUsername, allSpaceBindings) if userBinding == nil { // if PublicViewer is enabled, check if the Space is visibile to PublicViewer + // in case usersignup is the KubesawAuthenticatedUsername, then we already checked in the previous step if context.IsPublicViewerEnabled(ctx) && userSignup.CompliantUsername != toolchainv1alpha1.KubesawAuthenticatedUsername { userBinding = filterUserSpaceBinding(toolchainv1alpha1.KubesawAuthenticatedUsername, allSpaceBindings) } From 98bd360421dd9facf16d735d2f58a634918c25fc Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 13 Aug 2024 10:29:09 +0200 Subject: [PATCH 16/19] improve unit tests Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get_test.go | 26 ++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index 40f17e83..e81c911a 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -761,6 +761,7 @@ func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { fakeSignupService := fake.NewSignupService( newSignup("batman", "batman.space", true), newSignup("robin", "robin.space", true), + newSignup("gordon", "gordon.no-space", false), ) fakeClient := fake.InitClient(t, @@ -809,10 +810,15 @@ func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { expectedWorkspace: nil, }, "gordon can get robin workspace": { - username: "gordon.space", + username: "gordon.no-space", workspaceRequest: "robin", expectedWorkspace: publicRobinWS, }, + "gordon can not get batman workspace": { + username: "gordon.no-space", + workspaceRequest: "batman", + expectedWorkspace: nil, + }, } for k, tc := range tests { @@ -854,11 +860,12 @@ func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { } } -func TestSpaceListerGetWithBindingsWithPublicViewerEnabled(t *testing.T) { +func TestGetUserWorkspaceWithBindingsWithPublicViewerEnabled(t *testing.T) { fakeSignupService := fake.NewSignupService( newSignup("batman", "batman.space", true), newSignup("robin", "robin.space", true), + newSignup("gordon", "gordon.no-space", false), ) fakeClient := fake.InitClient(t, @@ -933,6 +940,21 @@ func TestSpaceListerGetWithBindingsWithPublicViewerEnabled(t *testing.T) { workspaceRequest: "batman", expectedWorkspace: nil, }, + "gordon can not get batman workspace": { + username: "gordon.no-space", + workspaceRequest: "batman", + expectedWorkspace: nil, + }, + "gordon can get robin workspace": { + username: "gordon.no-space", + workspaceRequest: "robin", + expectedWorkspace: func() *toolchainv1alpha1.Workspace { + batmansRobinWS := robinWS.DeepCopy() + batmansRobinWS.Status.Type = "" + batmansRobinWS.Status.Role = "viewer" + return batmansRobinWS + }(), + }, } for k, tc := range tests { From 6c0bbaf7c7db38626a05aa0e04e0ccae00113b00 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 13 Aug 2024 10:29:39 +0200 Subject: [PATCH 17/19] cleanup unit-tests Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get_test.go | 2 +- pkg/proxy/handlers/spacelister_list_test.go | 2 +- pkg/proxy/handlers/spacelister_test.go | 14 +------------- 3 files changed, 3 insertions(+), 15 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index e81c911a..d6046778 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -47,7 +47,7 @@ func TestSpaceListerGet(t *testing.T) { } func testSpaceListerGet(t *testing.T, publicViewerEnabled bool) { - fakeSignupService, fakeClient := buildSpaceListerFakes(t, publicViewerEnabled) + 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 0fb9c760..c07b4a09 100644 --- a/pkg/proxy/handlers/spacelister_list_test.go +++ b/pkg/proxy/handlers/spacelister_list_test.go @@ -25,7 +25,7 @@ import ( ) func TestSpaceListerList(t *testing.T) { - fakeSignupService, fakeClient := buildSpaceListerFakes(t, false) + fakeSignupService, fakeClient := buildSpaceListerFakes(t) t.Run("HandleSpaceListRequest", func(t *testing.T) { // given diff --git a/pkg/proxy/handlers/spacelister_test.go b/pkg/proxy/handlers/spacelister_test.go index 82f2854f..0049a35d 100644 --- a/pkg/proxy/handlers/spacelister_test.go +++ b/pkg/proxy/handlers/spacelister_test.go @@ -20,7 +20,7 @@ import ( spacetest "github.com/codeready-toolchain/toolchain-common/pkg/test/space" ) -func buildSpaceListerFakes(t *testing.T, publicViewerEnabled bool) (*fake.SignupService, *test.FakeClient) { +func buildSpaceListerFakes(t *testing.T) (*fake.SignupService, *test.FakeClient) { signups := []fake.SignupDef{ newSignup("dancelover", "dance.lover", true), newSignup("movielover", "movie.lover", true), @@ -34,12 +34,6 @@ 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), - ) - } fakeSignupService := fake.NewSignupService(signups...) // space that is not provisioned yet @@ -104,12 +98,6 @@ 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"), - ) - } fakeClient := fake.InitClient(t, objs...) return fakeSignupService, fakeClient From f0bf289f7535a6b63db98f053f73a89b6d4faef0 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 13 Aug 2024 11:12:59 +0200 Subject: [PATCH 18/19] Update pkg/proxy/handlers/spacelister_get_test.go Co-authored-by: Francisc Munteanu --- pkg/proxy/handlers/spacelister_get_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index d6046778..2328fcb6 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -809,7 +809,7 @@ func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { workspaceRequest: "batman", expectedWorkspace: nil, }, - "gordon can get robin workspace": { + "gordon can get robin workspace as public-viewer": { username: "gordon.no-space", workspaceRequest: "robin", expectedWorkspace: publicRobinWS, From a5541e26bec17d1df5618f4e5d276bb3b90dc9bb Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 13 Aug 2024 12:25:36 +0200 Subject: [PATCH 19/19] rollback unneeded changes Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_test.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_test.go b/pkg/proxy/handlers/spacelister_test.go index 0049a35d..f9804e74 100644 --- a/pkg/proxy/handlers/spacelister_test.go +++ b/pkg/proxy/handlers/spacelister_test.go @@ -21,7 +21,7 @@ import ( ) func buildSpaceListerFakes(t *testing.T) (*fake.SignupService, *test.FakeClient) { - signups := []fake.SignupDef{ + fakeSignupService := fake.NewSignupService( newSignup("dancelover", "dance.lover", true), newSignup("movielover", "movie.lover", true), newSignup("pandalover", "panda.lover", true), @@ -33,8 +33,7 @@ func buildSpaceListerFakes(t *testing.T) (*fake.SignupService, *test.FakeClient) newSignup("parentspace", "parent.space", true), newSignup("childspace", "child.space", true), newSignup("grandchildspace", "grandchild.space", true), - } - fakeSignupService := fake.NewSignupService(signups...) + ) // space that is not provisioned yet spaceNotProvisionedYet := fake.NewSpace("pandalover", "member-2", "pandalover") @@ -61,7 +60,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 - objs := []runtime.Object{ + fakeClient := fake.InitClient(t, // spaces fake.NewSpace("dancelover", "member-1", "dancelover"), fake.NewSpace("movielover", "member-1", "movielover"), @@ -75,8 +74,6 @@ 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"), @@ -97,8 +94,7 @@ func buildSpaceListerFakes(t *testing.T) (*fake.SignupService, *test.FakeClient) //nstemplatetier fake.NewBase1NSTemplateTier(), - } - fakeClient := fake.InitClient(t, objs...) + ) return fakeSignupService, fakeClient }