From c32a2fa3afa88c6ce9032b4f9733ded4a19f7e77 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 26 Aug 2024 11:20:36 +0200 Subject: [PATCH] add public-viewer support to SpaceLister's Get (#451) * add public-viewer support to pkg/configuration this commit is part of PR #443 Signed-off-by: Francesco Ilario * add public-viewer support to pkg/context this commit is part of PR #443 Signed-off-by: Francesco Ilario * add public-viewer support to SpaceLister's Get this commit is part of the effort of splitting PR #443 Signed-off-by: Francesco Ilario * simplify logs by using Errorf Signed-off-by: Francesco Ilario * improve logs Signed-off-by: Francesco Ilario * fix logs Signed-off-by: Francesco Ilario * remove unused parameter from getUserSpaceBinding Signed-off-by: Francesco Ilario * update comments Signed-off-by: Francesco Ilario * remove context's PublicViewerEnabled set Signed-off-by: Francesco Ilario * remove expectedErr Signed-off-by: Francesco Ilario * fix test case title Signed-off-by: Francesco Ilario * add missing tests Signed-off-by: Francesco Ilario * Update pkg/proxy/handlers/spacelister_get_test.go Co-authored-by: Alexey Kazakov * Update pkg/proxy/handlers/spacelister_get_test.go Co-authored-by: Alexey Kazakov * Update pkg/proxy/handlers/spacelister_get.go Co-authored-by: Francisc Munteanu * improve unit tests Signed-off-by: Francesco Ilario * cleanup unit-tests Signed-off-by: Francesco Ilario * Update pkg/proxy/handlers/spacelister_get_test.go Co-authored-by: Francisc Munteanu * rollback unneeded changes Signed-off-by: Francesco Ilario --------- Signed-off-by: Francesco Ilario Co-authored-by: Alexey Kazakov Co-authored-by: Francisc Munteanu --- pkg/proxy/handlers/spacelister_get.go | 85 ++++++- pkg/proxy/handlers/spacelister_get_test.go | 275 +++++++++++++++++++++ 2 files changed, 348 insertions(+), 12 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get.go b/pkg/proxy/handlers/spacelister_get.go index 0572f063..576ac5c9 100644 --- a/pkg/proxy/handlers/spacelister_get.go +++ b/pkg/proxy/handlers/spacelister_get.go @@ -8,6 +8,7 @@ import ( "time" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + "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" @@ -42,7 +43,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,34 +54,80 @@ 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(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 + } + + // 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(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 + } + if pvSb == nil { + 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 + } + ctx.Logger().Errorf("unauthorized access - there is no SpaceBinding present for the user %s and the workspace %s", userSignup.CompliantUsername, workspaceName) + } + + return userSpaceBinding, nil +} + +// 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) { 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) 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 { - // 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)) - ctx.Logger().Error(userBindingsErr) + userBindingsErr := fmt.Errorf("invalid number of SpaceBindings found for MUR:%s and Space:%s. Expected 1 got %d", compliantUsername, space.Name, len(userSpaceBindings)) 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 +153,17 @@ 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 + // 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) + } + + if userBinding == nil { + // let's only log the issue and consider this as not found + ctx.Logger().Errorf("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 +200,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..2328fcb6 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -32,6 +32,21 @@ import ( ) func TestSpaceListerGet(t *testing.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) memberFakeClient := fake.InitClient(t, @@ -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 @@ -603,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) @@ -677,6 +694,18 @@ func TestGetUserWorkspace(t *testing.T) { }, expectedWorkspace: nil, }, + "kubesaw-authenticated can not get robin workspace": { // Because public viewer feature is NOT enabled + username: toolchainv1alpha1.KubesawAuthenticatedUsername, + workspaceRequest: "robin", + expectedErr: "", + expectedWorkspace: nil, + }, + "batman can not get robin workspace": { // Because public viewer feature is NOT enabled + username: "batman.space", + workspaceRequest: "robin", + expectedErr: "", + expectedWorkspace: nil, + }, } for k, tc := range tests { @@ -726,3 +755,249 @@ func TestGetUserWorkspace(t *testing.T) { }) } } + +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, + // 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 + 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, + }, + "gordon can get robin workspace as public-viewer": { + 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 { + + 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 + 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 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, + // 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, + }, + "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 { + + 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) + }) + } +}