From 1cfcd94505166935aafdf2ee44d911be43717d3c Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Fri, 12 Jul 2024 16:41:42 +0200 Subject: [PATCH 01/70] fix: use UTC time in tests Signed-off-by: Francesco Ilario --- pkg/signup/service/signup_service_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/signup/service/signup_service_test.go b/pkg/signup/service/signup_service_test.go index c8d07e0c..08e5ead6 100644 --- a/pkg/signup/service/signup_service_test.go +++ b/pkg/signup/service/signup_service_test.go @@ -4,7 +4,6 @@ import ( "bytes" "errors" "fmt" - "github.com/codeready-toolchain/registration-service/pkg/util" "hash/crc32" "net/http" "net/http/httptest" @@ -17,6 +16,7 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/context" errors2 "github.com/codeready-toolchain/registration-service/pkg/errors" "github.com/codeready-toolchain/registration-service/pkg/signup/service" + "github.com/codeready-toolchain/registration-service/pkg/util" "github.com/codeready-toolchain/registration-service/test" "github.com/codeready-toolchain/registration-service/test/fake" @@ -1163,8 +1163,8 @@ func (s *TestSignupServiceSuite) TestGetSignupStatusOK() { require.Equal(t, "jsmith", response.Username) require.Equal(t, "ted", response.CompliantUsername) - require.Equal(t, mur.Status.ProvisionedTime.Format(time.RFC3339), response.StartDate) - require.Equal(t, us.Status.ScheduledDeactivationTimestamp.Format(time.RFC3339), response.EndDate) + require.Equal(t, mur.Status.ProvisionedTime.UTC().Format(time.RFC3339), response.StartDate) + require.Equal(t, us.Status.ScheduledDeactivationTimestamp.UTC().Format(time.RFC3339), response.EndDate) assert.True(t, response.Status.Ready) assert.Equal(t, "mur_ready_reason", response.Status.Reason) assert.Equal(t, "mur_ready_message", response.Status.Message) From 2c897f65ce88de6c1e30c1230ea71cd2fed5988c Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 18 Mar 2024 13:12:42 +0100 Subject: [PATCH 02/70] add public-viewer support This commit adds the notion of Public-Viewer to the Proxy as described in [JIRA ASC-532](https://issues.redhat.com/browse/ASC-532). Signed-off-by: Francesco Ilario --- go.mod | 4 +- go.sum | 8 +- pkg/application/service/services.go | 2 +- pkg/configuration/configuration.go | 4 + pkg/context/keys.go | 4 + pkg/context/public_viewer.go | 10 + pkg/log/log.go | 8 + pkg/proxy/handlers/spacelister_get.go | 77 +++++- pkg/proxy/handlers/spacelister_get_test.go | 267 ++++++++++++++++++- pkg/proxy/handlers/spacelister_list.go | 40 ++- pkg/proxy/handlers/spacelister_list_test.go | 81 +++++- pkg/proxy/handlers/spacelister_test.go | 26 +- pkg/proxy/proxy.go | 230 ++++++++++++++-- pkg/proxy/proxy_community_test.go | 278 ++++++++++++++++++++ pkg/proxy/proxy_test.go | 116 ++++---- pkg/proxy/service/cluster_service.go | 70 ++++- pkg/proxy/service/cluster_service_test.go | 46 ++-- pkg/server/in_cluster_application.go | 3 +- test/fake/proxy.go | 9 +- 19 files changed, 1142 insertions(+), 141 deletions(-) create mode 100644 pkg/context/public_viewer.go create mode 100644 pkg/proxy/proxy_community_test.go diff --git a/go.mod b/go.mod index c07e2a59..579d6483 100644 --- a/go.mod +++ b/go.mod @@ -4,8 +4,8 @@ go 1.20 require ( github.com/aws/aws-sdk-go v1.44.100 - github.com/codeready-toolchain/api v0.0.0-20240607180719-368c7afbaebe - github.com/codeready-toolchain/toolchain-common v0.0.0-20240613121043-7e6ef858cdff + github.com/codeready-toolchain/api v0.0.0-20240708122235-0af5a9a178bb + github.com/codeready-toolchain/toolchain-common v0.0.0-20240711082950-c7f9f4442ae0 github.com/go-logr/logr v1.4.1 github.com/gofrs/uuid v4.2.0+incompatible github.com/pkg/errors v0.9.1 diff --git a/go.sum b/go.sum index ae8b8e05..2b47df67 100644 --- a/go.sum +++ b/go.sum @@ -48,10 +48,10 @@ github.com/cloudflare/circl v1.1.0/go.mod h1:prBCrKB9DV4poKZY1l9zBXg2QJY7mvgRvtM github.com/cloudflare/circl v1.3.7 h1:qlCDlTPz2n9fu58M0Nh1J/JzcFpfgkFHHX3O35r5vcU= github.com/cloudflare/circl v1.3.7/go.mod h1:sRTcRWXGLrKw6yIGJ+l7amYJFfAXbZG0kBSc8r4zxgA= github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGXZJjfX53e64911xZQV5JYwmTeXPW+k8Sc= -github.com/codeready-toolchain/api v0.0.0-20240607180719-368c7afbaebe h1:l+KsEXkNe1mZ14Z/RaTgeUkEuX9r56mSZC6xlu5H6zY= -github.com/codeready-toolchain/api v0.0.0-20240607180719-368c7afbaebe/go.mod h1:ie9p4LenCCS0LsnbWp6/xwpFDdCWYE0KWzUO6Sk1g0E= -github.com/codeready-toolchain/toolchain-common v0.0.0-20240613121043-7e6ef858cdff h1:bVWL+2eayFKUnEzdEAwltPs+pzbGlGDSmrM3oOV2Ams= -github.com/codeready-toolchain/toolchain-common v0.0.0-20240613121043-7e6ef858cdff/go.mod h1:cyHrUfvBYEtsf+FbqQYmR9y0AQi9QAVtM3SUWLA5bd4= +github.com/codeready-toolchain/api v0.0.0-20240708122235-0af5a9a178bb h1:Wc9CMsv0ODZv9dM5qF3OI0mFDO95YNIXV/8oRvoz8aE= +github.com/codeready-toolchain/api v0.0.0-20240708122235-0af5a9a178bb/go.mod h1:ie9p4LenCCS0LsnbWp6/xwpFDdCWYE0KWzUO6Sk1g0E= +github.com/codeready-toolchain/toolchain-common v0.0.0-20240711082950-c7f9f4442ae0 h1:v7Z5i0JaF1H9SYxK/uEjWgH8Vpm4Eg3OJep/Pl/2iyM= +github.com/codeready-toolchain/toolchain-common v0.0.0-20240711082950-c7f9f4442ae0/go.mod h1:8M9k7w2VSyRKSK6P08Jo2ddW3uyGgxCcSitnYa3HK9o= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= diff --git a/pkg/application/service/services.go b/pkg/application/service/services.go index 4d15b6b0..2602dccc 100644 --- a/pkg/application/service/services.go +++ b/pkg/application/service/services.go @@ -38,7 +38,7 @@ type VerificationService interface { } type MemberClusterService interface { - GetClusterAccess(userID, username, workspace, proxyPluginName string) (*access.ClusterAccess, error) + GetClusterAccess(userID, username, workspace, proxyPluginName string, publicViewerEnabled bool) (*access.ClusterAccess, error) } type Services interface { 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/context/keys.go b/pkg/context/keys.go index 9477de21..4ae47251 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 content 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..f0b14e03 --- /dev/null +++ b/pkg/context/public_viewer.go @@ -0,0 +1,10 @@ +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/log/log.go b/pkg/log/log.go index f2107f8e..7ab56dee 100644 --- a/pkg/log/log.go +++ b/pkg/log/log.go @@ -149,6 +149,14 @@ func (l *Logger) InfoEchof(ctx echo.Context, msg string, args ...string) { ctxFields = append(ctxFields, "url") ctxFields = append(ctxFields, ctx.Request().URL) + if impersonateUser, ok := ctx.Get(context.ImpersonateUser).(string); ok { + ctxFields = append(ctxFields, "impersonate-user", impersonateUser) + } + + if publicViewerEnabled, ok := ctx.Get(context.PublicViewerEnabled).(bool); ok { + ctxFields = append(ctxFields, "public-viewer-enabled", publicViewerEnabled) + } + l.infof(ctxFields, msg, args...) } diff --git a/pkg/proxy/handlers/spacelister_get.go b/pkg/proxy/handlers/spacelister_get.go index 53b98930..6925fce3 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" @@ -43,7 +44,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 { @@ -54,6 +55,56 @@ 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) + 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) { + publicViewerUserSignup := signup.Signup{ + Name: toolchainv1alpha1.KubesawAuthenticatedUsername, + CompliantUsername: toolchainv1alpha1.KubesawAuthenticatedUsername, + } + pvSb, err := getUserSpaceBinding(ctx, spaceLister, space, &publicViewerUserSignup) + if err != nil { + ctx.Logger().Error(fmt.Sprintf("error checking if SpaceBinding is present for user %s and the workspace %s", publicViewerUserSignup.CompliantUsername, 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", userSignup.CompliantUsername, 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. +func getUserSpaceBinding(ctx echo.Context, spaceLister *SpaceLister, space *toolchainv1alpha1.Space, userSignup *signup.Signup) (*toolchainv1alpha1.SpaceBinding, error) { // recursively get all the spacebindings for the current workspace listSpaceBindingsFunc := func(spaceName string) ([]toolchainv1alpha1.SpaceBinding, error) { spaceSelector, err := labels.NewRequirement(toolchainv1alpha1.SpaceBindingSpaceLabelKey, selection.Equals, []string{spaceName}) @@ -74,7 +125,6 @@ func GetUserWorkspace(ctx echo.Context, spaceLister *SpaceLister, workspaceName } 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)) return nil, nil } @@ -84,10 +134,10 @@ func GetUserWorkspace(ctx echo.Context, spaceLister *SpaceLister, workspaceName 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 { @@ -116,9 +166,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. @@ -155,6 +212,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..dfbb7981 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -31,8 +31,16 @@ import ( runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" ) -func TestSpaceListerGet(t *testing.T) { - fakeSignupService, fakeClient := buildSpaceListerFakes(t) +func TestSpaceListerGetCommonCommunityEnabled(t *testing.T) { + testSpaceListerGet(t, true) +} + +func TestSpaceListerGetCommonCommunityDisabled(t *testing.T) { + testSpaceListerGet(t, false) +} + +func testSpaceListerGet(t *testing.T, publicViewerEnabled bool) { + fakeSignupService, fakeClient := buildSpaceListerFakes(t, publicViewerEnabled) memberFakeClient := fake.InitClient(t, // spacebinding requests @@ -564,6 +572,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 +735,257 @@ func TestGetUserWorkspace(t *testing.T) { }) } } + +func TestSpaceListerGetCommunityEnabled(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) + + tests := map[string]struct { + username string + expectedErr string + workspaceRequest string + expectedWorkspace *toolchainv1alpha1.Workspace + overrideInformerFunc func() service.InformerService + overrideSignupFunc func(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error) + }{ + "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, + expectedErr: "", + }, + } + + for k, tc := range tests { + + t.Run(k, func(t *testing.T) { + // given + signupProvider := fakeSignupService.GetSignupFromInformer + if tc.overrideSignupFunc != nil { + signupProvider = tc.overrideSignupFunc + } + informerFunc := fake.GetInformerService(fakeClient) + if tc.overrideInformerFunc != nil { + informerFunc = tc.overrideInformerFunc + } + + 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 TestSpaceListerGetWithBindingsCommunityEnabled(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 + expectedErr string + workspaceRequest string + expectedWorkspace *toolchainv1alpha1.Workspace + overrideInformerFunc func() service.InformerService + overrideSignupFunc func(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error) + }{ + "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, + expectedErr: "", + }, + } + + for k, tc := range tests { + + t.Run(k, func(t *testing.T) { + // given + signupProvider := fakeSignupService.GetSignupFromInformer + if tc.overrideSignupFunc != nil { + signupProvider = tc.overrideSignupFunc + } + informerFunc := fake.GetInformerService(fakeClient) + if tc.overrideInformerFunc != nil { + informerFunc = tc.overrideInformerFunc + } + + 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 + 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 + } + }) + } +} diff --git a/pkg/proxy/handlers/spacelister_list.go b/pkg/proxy/handlers/spacelister_list.go index bc2af8fe..3199c4d1 100644 --- a/pkg/proxy/handlers/spacelister_list.go +++ b/pkg/proxy/handlers/spacelister_list.go @@ -6,15 +6,17 @@ 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 { @@ -40,19 +42,38 @@ func ListUserWorkspaces(ctx echo.Context, spaceLister *SpaceLister) ([]toolchain } // signup is not ready if signup == nil { - return []toolchainv1alpha1.Workspace{}, nil + return nil, nil + } + + // get MUR Names + murNames := getMURNamesForList(ctx, signup) + if len(murNames) == 0 { + return nil, nil } - murName := signup.CompliantUsername // 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 MasterUserRecord names to use in listing Workspaces. +// If PublicViewer is enabled, the list will contain at least 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{ @@ -67,13 +88,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) { - murSelector, err := labels.NewRequirement(toolchainv1alpha1.SpaceBindingMasterUserRecordLabelKey, selection.Equals, []string{murName}) +func listSpaceBindingsForUsers(spaceLister *SpaceLister, murNames []string) ([]toolchainv1alpha1.SpaceBinding, error) { + murSelector, err := labels.NewRequirement(toolchainv1alpha1.SpaceBindingMasterUserRecordLabelKey, selection.In, murNames) if err != nil { return nil, err } - requirements := []labels.Requirement{*murSelector} - return spaceLister.GetInformerServiceFunc().ListSpaceBindings(requirements...) + 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..0d1f3ce3 100644 --- a/pkg/proxy/handlers/spacelister_list_test.go +++ b/pkg/proxy/handlers/spacelister_list_test.go @@ -24,8 +24,86 @@ import ( "github.com/codeready-toolchain/registration-service/test/fake" ) +func TestSpaceListerListCommunity(t *testing.T) { + publicViewerEnabled := true + fakeSignupService, fakeClient := buildSpaceListerFakes(t, publicViewerEnabled) + 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, "communityspace", "viewer", false), + workspaceFor(t, fakeClient, "dancelover", "admin", true), + workspaceFor(t, fakeClient, "movielover", "other", false), + }, + expectedErr: "", + }, + } + + t.Run("HandleSpaceListRequest", func(t *testing.T) { + for k, tc := range tests { + t.Run(k, func(t *testing.T) { + // given + signupProvider := fakeSignupService.GetSignupFromInformer + if tc.overrideSignupFunc != nil { + signupProvider = tc.overrideSignupFunc + } + + informerFunc := fake.GetInformerService(fakeClient) + if tc.overrideInformerFunc != nil { + informerFunc = tc.overrideInformerFunc + } + + 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, 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) + } + } + }) + } + }) +} + func TestSpaceListerList(t *testing.T) { - fakeSignupService, fakeClient := buildSpaceListerFakes(t) + publicViewerEnabled := false + fakeSignupService, fakeClient := buildSpaceListerFakes(t, publicViewerEnabled) t.Run("HandleSpaceListRequest", func(t *testing.T) { // given @@ -124,6 +202,7 @@ func TestSpaceListerList(t *testing.T) { ctx.Set(rcontext.RequestReceivedTime, time.Now()) // when + ctx.Set(rcontext.PublicViewerEnabled, publicViewerEnabled) err := handlers.HandleSpaceListRequest(s)(ctx) // then 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 } diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index d950c1bb..581ff967 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -26,6 +26,7 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/proxy/access" "github.com/codeready-toolchain/registration-service/pkg/proxy/handlers" "github.com/codeready-toolchain/registration-service/pkg/proxy/metrics" + "github.com/codeready-toolchain/registration-service/pkg/signup" commoncluster "github.com/codeready-toolchain/toolchain-common/pkg/cluster" "github.com/labstack/echo/v4" "github.com/labstack/echo/v4/middleware" @@ -141,6 +142,7 @@ func (p *Proxy) StartProxy(port string) *http.Server { // Space lister routes wg.GET("/:workspace", handlers.HandleSpaceGetRequest(p.spaceLister, p.getMembersFunc)) wg.GET("", handlers.HandleSpaceListRequest(p.spaceLister)) + router.GET(proxyHealthEndpoint, p.health) // SSO routes. Used by web login (oc login -w). // Here is the expected flow for the "oc login -w" command: @@ -264,6 +266,7 @@ func (p *Proxy) health(ctx echo.Context) error { } func (p *Proxy) processRequest(ctx echo.Context) (string, *access.ClusterAccess, error) { + // retrieve required information from the HTTP request userID, _ := ctx.Get(context.SubKey).(string) username, _ := ctx.Get(context.UsernameKey).(string) proxyPluginName, workspaceName, err := getWorkspaceContext(ctx.Request()) @@ -271,44 +274,213 @@ func (p *Proxy) processRequest(ctx echo.Context) (string, *access.ClusterAccess, return "", nil, crterrors.NewBadRequest("unable to get workspace context", err.Error()) } - ctx.Set(context.WorkspaceKey, workspaceName) // set workspace context for logging - cluster, err := p.app.MemberClusterService().GetClusterAccess(userID, username, workspaceName, proxyPluginName) + // set workspace context for logging + ctx.Set(context.WorkspaceKey, workspaceName) + + // if the target workspace is NOT explicitly declared in the HTTP request, + // process the request against the user's home workspace + if workspaceName == "" { + cluster, err := p.processHomeWorkspaceRequest(ctx, userID, username, proxyPluginName) + if err != nil { + return "", nil, err + } + return proxyPluginName, cluster, nil + } + + // if the target workspace is explicitly declared in the HTTP request, + // process the request against the declared workspace + cluster, err := p.processWorkspaceRequest(ctx, userID, username, workspaceName, proxyPluginName) + if err != nil { + return "", nil, err + } + return proxyPluginName, cluster, nil +} + +// processHomeWorkspaceRequest process an HTTP Request targeting the user's home workspace. +func (p *Proxy) processHomeWorkspaceRequest(ctx echo.Context, userID, username, proxyPluginName string) (*access.ClusterAccess, error) { + // retrieves the ClusterAccess for the user and their home workspace + cluster, err := p.app.MemberClusterService().GetClusterAccess(userID, username, "", proxyPluginName, false) if err != nil { - return "", nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) + return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) } - // before proxying the request, verify that the user has a spacebinding for the workspace and that the namespace (if any) belongs to the workspace - var workspaces []toolchainv1alpha1.Workspace - if workspaceName != "" { - // when a workspace name was provided - // validate that the user has access to the workspace by getting all spacebindings recursively, starting from this workspace and going up to the parent workspaces till the "root" of the workspace tree. - workspace, err := handlers.GetUserWorkspace(ctx, p.spaceLister, workspaceName) + // list all workspaces the user has access to + workspaces, err := handlers.ListUserWorkspaces(ctx, p.spaceLister) + if err != nil { + return nil, crterrors.NewInternalError(errs.New("unable to retrieve user workspaces"), err.Error()) + } + + // check whether the user's has access to the home workspace + // and whether the requestedNamespace -if any- exists in the workspace. + requestedNamespace := namespaceFromCtx(ctx) + if err := validateWorkspaceRequest("", requestedNamespace, workspaces); err != nil { + return nil, crterrors.NewForbiddenError("invalid workspace request", err.Error()) + } + + // return the cluster access + return cluster, nil +} + +// processWorkspaceRequest process an HTTP Request targeting a specific workspace. +func (p *Proxy) processWorkspaceRequest(ctx echo.Context, userID, username, workspaceName, proxyPluginName string) (*access.ClusterAccess, error) { + // check if the user has access to the target workspace + if err := p.validateSpaceAccess(ctx, userID, username, workspaceName); err != nil { + return nil, err + } + + // before proxying the request, verify that the user has a spacebinding for + // the workspace and that the namespace -if any- belongs to the workspace + workspaces, err := p.listUserWorkspaces(ctx, workspaceName) + if err != nil { + return nil, err + } + + // check whether the user's has access to the home workspace + // and whether the requestedNamespace -if any- exists in the workspace. + requestedNamespace := namespaceFromCtx(ctx) + if err := validateWorkspaceRequest(workspaceName, requestedNamespace, workspaces); err != nil { + return nil, crterrors.NewForbiddenError("invalid workspace request", err.Error()) + } + + // retrieve the ClusterAccess for the user and the target workspace + return p.getClusterAccess(ctx, userID, username, workspaceName, proxyPluginName, workspaces) +} + +// validateSpaceAccess checks whether the user has access to the target workspace and that the Space exists. +func (p *Proxy) validateSpaceAccess(ctx echo.Context, userID, username, workspaceName string) error { + if err := p.validateUserAccess(ctx, userID, username); err != nil { + return crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) + } + if err := p.validateSpace(workspaceName); err != nil { + return crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) + } + + return nil +} + +// validateSpace checks whether the Space exists. +func (p *Proxy) validateSpace(workspaceName string) error { + if _, err := p.app.InformerService().GetSpace(workspaceName); err != nil { + // log the actual error but do not return it so that it doesn't reveal information about a space that may not belong to the requestor + log.Error(nil, err, "unable to get target cluster for workspace "+workspaceName) + return fmt.Errorf("the requested space is not available") + } + return nil +} + +// validateUserAccess checks whether the user is Approved, if they are not an error is returned. +// If public-viewer is enabled, user validation is skipped. +func (p *Proxy) validateUserAccess(ctx echo.Context, userID, username string) error { + // skip if public-viewer is enabled: read-only operations on community workspaces are always permitted. + if context.IsPublicViewerEnabled(ctx) { + return nil + } + + // retrieve the UserSignup for the requesting user. + // + // UserSignup complete status is not checked, since it might cause the proxy blocking the request + // and returning an error when quick transitions from ready to provisioning are happening. + userSignup, err := p.app.SignupService().GetSignupFromInformer(nil, userID, username, false) + if err != nil { + return err + } + + // if the UserSignup is nil or has NOT the CompliantUsername set, + // it means that MUR was NOT created and useraccount is NOT provisioned yet + // TODO(@filariow): should we also check if the user is Banned? + if userSignup == nil || userSignup.CompliantUsername == "" { + cause := errs.New("user is not provisioned (yet)") + log.Error(nil, cause, fmt.Sprintf("signup object: %+v", userSignup)) + return cause + } + return nil +} + +// getClusterAccess retrieves the access to the cluster hosting the requested workspace, +// if the user has access to it. +// Access can be either direct (an SpaceBinding linking the user to the workspace exists) +// or community (a SpaceBinding linking the PublicViewer user to the workspace exists). +func (p *Proxy) getClusterAccess(ctx echo.Context, userID, username, workspaceName, proxyPluginName string, workspaces []toolchainv1alpha1.Workspace) (*access.ClusterAccess, error) { + // retrieve by name the workspace from the list of workspaces the user has access to. + w := findWorkspaceByName(workspaceName, workspaces) + if w == nil { + return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), "workspace not found") + } + + // retrieve the requesting user's UserSignup + signup, err := p.spaceLister.GetSignupFunc(nil, userID, username, false) + if err != nil { + return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), "user not found") + } + + // if PublicViewer is enabled and user has no direct access to the workspace, proceed as PublicViewer + publicViewerEnabled := context.IsPublicViewerEnabled(ctx) + if publicViewerEnabled && !hasDirectAccess(signup, w) { + cluster, err := p.app.MemberClusterService().GetClusterAccess( + toolchainv1alpha1.KubesawAuthenticatedUsername, + toolchainv1alpha1.KubesawAuthenticatedUsername, + workspaceName, + proxyPluginName, + publicViewerEnabled) if err != nil { - return "", nil, crterrors.NewInternalError(errs.New("unable to retrieve user workspaces"), err.Error()) + return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) } - if workspace == nil { - // not found - return "", nil, crterrors.NewForbiddenError("invalid workspace request", fmt.Sprintf("access to workspace '%s' is forbidden", workspaceName)) - } - // workspace was found means we can forward the request - workspaces = []toolchainv1alpha1.Workspace{*workspace} - } else { - // list all workspaces - workspaces, err = handlers.ListUserWorkspaces(ctx, p.spaceLister) - if err != nil { - return "", nil, crterrors.NewInternalError(errs.New("unable to retrieve user workspaces"), err.Error()) + return cluster, nil + } + + // retrieve the ClusterAccess for the cluster hosting the workspace and the given user. + cluster, err := p.app.MemberClusterService().GetClusterAccess(userID, username, workspaceName, proxyPluginName, publicViewerEnabled) + if err != nil { + return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) + } + return cluster, nil +} + +// hasDirectAccess checks if an UserSignup has access to a workspace. +// Workspace's bindings are obtained from its `status.bindings` property. +func hasDirectAccess(signup *signup.Signup, workspace *toolchainv1alpha1.Workspace) bool { + if signup == nil || workspace == nil { + return false + } + + for _, b := range workspace.Status.Bindings { + if b.MasterUserRecord == signup.CompliantUsername { + return true } } - requestedNamespace := namespaceFromCtx(ctx) - if err := validateWorkspaceRequest(workspaceName, requestedNamespace, workspaces); err != nil { - return "", nil, crterrors.NewForbiddenError("invalid workspace request", err.Error()) + return false +} + +// findWorkspaceByName finds a workspace by name in a list of Workspaces. +func findWorkspaceByName(workspaceName string, workspaces []toolchainv1alpha1.Workspace) *toolchainv1alpha1.Workspace { + for _, w := range workspaces { + if w.Name == workspaceName { + w := w // TODO We won't need it after upgrading to go 1.22: https://go.dev/blog/loopvar-preview + return &w + } } + return nil +} - return proxyPluginName, cluster, nil +func (p *Proxy) listUserWorkspaces(ctx echo.Context, workspaceName string) ([]toolchainv1alpha1.Workspace, error) { + // when a workspace name was provided + // validate that the user has access to the workspace by getting all spacebindings recursively, starting from this workspace and going up to the parent workspaces till the "root" of the workspace tree. + workspace, err := handlers.GetUserWorkspaceWithBindings(ctx, p.spaceLister, workspaceName, p.getMembersFunc) + if err != nil { + return nil, crterrors.NewInternalError(errs.New("unable to retrieve user workspaces"), err.Error()) + } + if workspace == nil { + // not found + return nil, crterrors.NewForbiddenError("invalid workspace request", fmt.Sprintf("access to workspace '%s' is forbidden", workspaceName)) + } + // workspace was found means we can forward the request + return []toolchainv1alpha1.Workspace{*workspace}, nil } func (p *Proxy) handleRequestAndRedirect(ctx echo.Context) error { requestReceivedTime := ctx.Get(context.RequestReceivedTime).(time.Time) + publicViewerEnabled := configuration.GetRegistrationServiceConfig().PublicViewerEnabled() + ctx.Set(context.PublicViewerEnabled, publicViewerEnabled) proxyPluginName, cluster, err := p.processRequest(ctx) if err != nil { p.metrics.RegServProxyAPIHistogramVec.WithLabelValues(fmt.Sprintf("%d", http.StatusNotAcceptable), metrics.MetricLabelRejected).Observe(time.Since(requestReceivedTime).Seconds()) @@ -464,11 +636,16 @@ func extractUserToken(req *http.Request) (string, error) { func (p *Proxy) newReverseProxy(ctx echo.Context, target *access.ClusterAccess, isPlugin bool) *httputil.ReverseProxy { req := ctx.Request() targetQuery := target.APIURL().RawQuery + username, _ := ctx.Get(context.UsernameKey).(string) + ctx.Set(context.ImpersonateUser, target.Username()) + director := func(req *http.Request) { origin := req.URL.String() req.URL.Scheme = target.APIURL().Scheme req.URL.Host = target.APIURL().Host req.URL.Path = singleJoiningSlash(target.APIURL().Path, req.URL.Path) + req.Header.Set("X-SSO-User", username) + if isPlugin { // for non k8s clients testing, like vanilla http clients accessing plugin proxy flows, testing has proven that the request // host needs to be updated in addition to the URL in order to have the reverse proxy contact the openshift @@ -632,6 +809,9 @@ func replaceTokenInWebsocketRequest(req *http.Request, newToken string) { req.Header.Set(ph, strings.Join(protocols, ",")) } +// validateWorkspaceRequest checks whether the requested workspace is in the list of workspaces the user has visibility on (retrieved via the spaceLister). +// If `requestedWorkspace` is zero, this function looks for the home workspace (the one with `status.Type` set to `home`). +// If `requestedNamespace` is NOT zero, this function checks if the namespace exists in the workspace. func validateWorkspaceRequest(requestedWorkspace, requestedNamespace string, workspaces []toolchainv1alpha1.Workspace) error { // check workspace access isHomeWSRequested := requestedWorkspace == "" diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go new file mode 100644 index 00000000..8e2ed5e6 --- /dev/null +++ b/pkg/proxy/proxy_community_test.go @@ -0,0 +1,278 @@ +package proxy + +import ( + "context" + "fmt" + "log" + "net/http" + "net/http/httptest" + "time" + + appservice "github.com/codeready-toolchain/registration-service/pkg/application/service" + "github.com/codeready-toolchain/registration-service/pkg/auth" + "github.com/codeready-toolchain/registration-service/pkg/proxy/handlers" + "github.com/codeready-toolchain/registration-service/pkg/signup" + "github.com/codeready-toolchain/registration-service/test/fake" + "github.com/google/uuid" + + toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + testconfig "github.com/codeready-toolchain/toolchain-common/pkg/test/config" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/labels" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func (s *TestProxySuite) TestProxyCommunityEnabled() { + // given + + port := "30456" + + env := s.DefaultConfig().Environment() + defer s.SetConfig(testconfig.RegistrationService(). + Environment(env)) + s.SetConfig(testconfig.RegistrationService(). + Environment(string(testconfig.E2E))) // We use e2e-test environment just to be able to re-use token generation + _, err := auth.InitializeDefaultTokenParser() + require.NoError(s.T(), err) + + for _, environment := range []testconfig.EnvName{testconfig.E2E, testconfig.Dev, testconfig.Prod} { + s.Run("for environment "+string(environment), func() { + // spin up proxy + s.SetConfig( + testconfig.RegistrationService().Environment(string(environment)), + testconfig.PublicViewerConfig(true), + ) + fakeApp := &fake.ProxyFakeApp{} + p, server := s.spinUpProxy(fakeApp, port) + defer func() { + _ = server.Close() + }() + + // wait for proxy to be alive + s.Run("is alive", func() { + s.waitForProxyToBeAlive(port) + }) + s.Run("health check ok", func() { + s.checkProxyIsHealthy(port) + }) + + // run community tests + s.checkProxyCommunityOK(fakeApp, p, port) + }) + } +} + +func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Proxy, port string) { + s.Run("successfully proxy", func() { + owner := uuid.New() + communityUser := uuid.New() + httpTestServerResponse := "my response" + + // Start the member-2 API Server + testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + // Set the Access-Control-Allow-Origin header to make sure it's overridden by the proxy response modifier + w.Header().Set("Access-Control-Allow-Origin", "dummy") + w.WriteHeader(http.StatusOK) + _, err := w.Write([]byte(httpTestServerResponse)) + require.NoError(s.T(), err) + })) + defer testServer.Close() + + type testCase struct { + ProxyRequestMethod string + ProxyRequestHeaders http.Header + ExpectedAPIServerRequestHeaders http.Header + ExpectedProxyResponseStatus int + RequestPath string + ExpectedResponse string + } + + tests := map[string]testCase{ + // Given smith2 owns a workspace named communityspace + // And communityspace is publicly visible (shared with PublicViewer) + // When smith2 requests the list of pods in workspace communityspace + // Then the request is forwarded from the proxy + // And the request impersonates smith2 + // And the request's X-SSO-User Header is set to smith2's ID + // And the request is successful + "plain http actual request as owner": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(owner)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {"smith2"}, + "X-SSO-User": {"username-" + owner.String()}, + }, + ExpectedProxyResponseStatus: http.StatusOK, + RequestPath: fmt.Sprintf("http://localhost:%s/workspaces/communityspace/api/communityspace/pods", port), + ExpectedResponse: httpTestServerResponse, + }, + // Given smith2 owns a workspace named communityspace + // And communityspace is publicly visible (shared with PublicViewer) + // And a user named communityuser exists + // And smith2's communityspace is not directly shared with communityuser + // When communityuser requests the list of pods in workspace communityspace + // Then the request is forwarded from the proxy + // And the request impersonates the PublicViewer + // And the request's X-SSO-User Header is set to communityuser's ID + // And the request is successful + "plain http actual request as community user": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(communityUser)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {toolchainv1alpha1.KubesawAuthenticatedUsername}, + "X-SSO-User": {"username-" + communityUser.String()}, + }, + ExpectedProxyResponseStatus: http.StatusOK, + RequestPath: fmt.Sprintf("http://localhost:%s/workspaces/communityspace/api/communityspace/pods", port), + ExpectedResponse: httpTestServerResponse, + }, + // Given user alice exists + // And alice owns a private workspace + // When smith2 requests the list of pods in alice's workspace + // Then the request is forwarded from the proxy + // And the request impersonates smith2 + // And the request's X-SSO-User Header is set to smith2's ID + // And the request is NOT successful + "plain http actual request as not owner to private workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(owner)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {"smith2"}, + "X-SSO-User": {"username-" + owner.String()}, + }, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: fmt.Sprintf("http://localhost:%s/workspaces/alice-private/api/alice-private/pods", port), + ExpectedResponse: "invalid workspace request: access to workspace 'alice-private' is forbidden", + }, + } + + for k, tc := range tests { + s.Run(k, func() { + + // given + fakeApp.Err = nil + + testServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + // Set the Access-Control-Allow-Origin header to make sure it's overridden by the proxy response modifier + w.Header().Set("Access-Control-Allow-Origin", "dummy") + w.WriteHeader(http.StatusOK) + _, err := w.Write([]byte("my response")) + require.NoError(s.T(), err) + for hk, hv := range tc.ExpectedAPIServerRequestHeaders { + require.Len(s.T(), r.Header.Values(hk), len(hv)) + for i := range hv { + assert.Equal(s.T(), hv[i], r.Header.Values(hk)[i], "header %s", hk) + } + } + }) + fakeApp.SignupServiceMock = fake.NewSignupService( + fake.Signup(owner.String(), &signup.Signup{ + Name: "smith2", + APIEndpoint: testServer.URL, + ClusterName: "member-2", + CompliantUsername: "smith2", + Username: "smith2@", + Status: signup.Status{ + Ready: true, + }, + }), + fake.Signup(communityUser.String(), &signup.Signup{ + Name: "communityUser", + APIEndpoint: testServer.URL, + ClusterName: "member-2", + CompliantUsername: "communityuser", + Username: "communityUser@", + Status: signup.Status{ + Ready: true, + }, + }), + fake.Signup(communityUser.String(), &signup.Signup{ + Name: "alice", + APIEndpoint: testServer.URL, + ClusterName: "member-2", + CompliantUsername: "alice", + Username: "alice@", + Status: signup.Status{ + Ready: true, + }, + }), + ) + s.Application.MockSignupService(fakeApp.SignupServiceMock) + inf := fake.NewFakeInformer() + inf.GetSpaceFunc = func(name string) (*toolchainv1alpha1.Space, error) { + switch name { + case "communityspace": + return fake.NewSpace("communityspace", "member-2", "smith2"), nil + case "alice-private": + return fake.NewSpace("alice-private", "member-2", "alice"), nil + } + return nil, fmt.Errorf("space not found error") + } + + sbmycoolSmith2 := fake.NewSpaceBinding("communityspace-smith2", "smith2", "communityspace", "admin") + commSpacePublicViewer := fake.NewSpaceBinding("communityspace-publicviewer", toolchainv1alpha1.KubesawAuthenticatedUsername, "communityspace", "viewer") + alicePrivate := fake.NewSpaceBinding("alice-default", "alice", "alice-default", "admin") + + cli := fake.InitClient(s.T(), sbmycoolSmith2, commSpacePublicViewer, alicePrivate) + inf.ListSpaceBindingFunc = func(reqs ...labels.Requirement) ([]toolchainv1alpha1.SpaceBinding, error) { + sbs := toolchainv1alpha1.SpaceBindingList{} + opts := &client.ListOptions{ + LabelSelector: labels.NewSelector().Add(reqs...), + } + log.Printf("received reqs: %v", reqs) + if err := cli.Client.List(context.TODO(), &sbs, opts); err != nil { + return nil, err + } + log.Printf("returning sbs: %v", sbs.Items) + return sbs.Items, nil + } + inf.GetProxyPluginConfigFunc = func(_ string) (*toolchainv1alpha1.ProxyPlugin, error) { + return nil, fmt.Errorf("proxy plugin not found") + } + inf.GetNSTemplateTierFunc = func(_ string) (*toolchainv1alpha1.NSTemplateTier, error) { + return fake.NewBase1NSTemplateTier(), nil + } + s.Application.MockInformerService(inf) + fakeApp.MemberClusterServiceMock = s.newMemberClusterServiceWithMembers(testServer.URL) + fakeApp.InformerServiceMock = inf + + p.spaceLister = &handlers.SpaceLister{ + GetSignupFunc: fakeApp.SignupServiceMock.GetSignupFromInformer, + GetInformerServiceFunc: func() appservice.InformerService { + return inf + }, + ProxyMetrics: p.metrics, + } + + // prepare request + req, err := http.NewRequest(tc.ProxyRequestMethod, tc.RequestPath, nil) + require.NoError(s.T(), err) + require.NotNil(s.T(), req) + + for hk, hv := range tc.ProxyRequestHeaders { + for _, v := range hv { + req.Header.Add(hk, v) + } + } + + // when + client := http.Client{Timeout: 3 * time.Second} + resp, err := client.Do(req) + + // then + require.NoError(s.T(), err) + require.NotNil(s.T(), resp) + defer resp.Body.Close() + assert.Equal(s.T(), tc.ExpectedProxyResponseStatus, resp.StatusCode) + s.assertResponseBody(resp, tc.ExpectedResponse) + }) + } + }) +} diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index d465cfda..b530b710 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -27,6 +27,7 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/signup" "github.com/codeready-toolchain/registration-service/test" "github.com/codeready-toolchain/registration-service/test/fake" + "github.com/google/uuid" "github.com/prometheus/client_golang/prometheus" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" @@ -35,7 +36,6 @@ import ( authsupport "github.com/codeready-toolchain/toolchain-common/pkg/test/auth" testconfig "github.com/codeready-toolchain/toolchain-common/pkg/test/config" - "github.com/google/uuid" routev1 "github.com/openshift/api/route/v1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -72,66 +72,82 @@ func (s *TestProxySuite) TestProxy() { s.SetConfig(testconfig.RegistrationService(). Environment(string(environment))) fakeApp := &fake.ProxyFakeApp{} - proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) - p, err := newProxyWithClusterClient(fakeApp, nil, proxyMetrics, proxytest.NewGetMembersFunc(fake.InitClient(s.T()))) - require.NoError(s.T(), err) - - server := p.StartProxy(DefaultPort) - require.NotNil(s.T(), server) + p, server := s.spinUpProxy(fakeApp, DefaultPort) defer func() { _ = server.Close() }() - // Wait up to N seconds for the Proxy server to start - ready := false - sec := 10 - for i := 0; i < sec; i++ { - log.Println("Checking if Proxy is started...") - req, err := http.NewRequest("GET", "http://localhost:8081/api/mycoolworkspace/pods", nil) - require.NoError(s.T(), err) - require.NotNil(s.T(), req) - resp, err := http.DefaultClient.Do(req) - if err != nil { - time.Sleep(time.Second) - continue - } - _, _ = io.Copy(io.Discard, resp.Body) - _ = resp.Body.Close() - if resp.StatusCode != http.StatusUnauthorized { - // The server may be running but still not fully ready to accept requests - time.Sleep(time.Second) - continue - } - // Server is up and running! - ready = true - break - } - require.True(s.T(), ready, "Proxy is not ready after %d seconds", sec) - + s.Run("is alive", func() { + s.waitForProxyToBeAlive(DefaultPort) + }) s.Run("health check ok", func() { - req, err := http.NewRequest("GET", "http://localhost:8081/proxyhealth", nil) - require.NoError(s.T(), err) - require.NotNil(s.T(), req) - - // when - resp, err := http.DefaultClient.Do(req) - - // then - require.NoError(s.T(), err) - require.NotNil(s.T(), resp) - defer resp.Body.Close() - assert.Equal(s.T(), http.StatusOK, resp.StatusCode) - s.assertResponseBody(resp, `{"alive": true}`) + s.checkProxyIsHealthy(DefaultPort) }) s.checkPlainHTTPErrors(fakeApp) s.checkWebsocketsError() s.checkWebLogin() - s.checkProxyOK(fakeApp, p) + s.checkProxyOK(fakeApp, p, false) }) } } +func (s *TestProxySuite) spinUpProxy(fakeApp *fake.ProxyFakeApp, port string) (*Proxy, *http.Server) { + proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) + p, err := newProxyWithClusterClient( + fakeApp, nil, proxyMetrics, proxytest.NewGetMembersFunc(fake.InitClient(s.T()))) + require.NoError(s.T(), err) + + server := p.StartProxy(port) + require.NotNil(s.T(), server) + + return p, server +} + +func (s *TestProxySuite) waitForProxyToBeAlive(port string) { + // Wait up to N seconds for the Proxy server to start + ready := false + sec := 10 + for i := 0; i < sec; i++ { + log.Println("Checking if Proxy is started...") + req, err := http.NewRequest("GET", fmt.Sprintf("http://localhost:%s/api/mycoolworkspace/pods", port), nil) + require.NoError(s.T(), err) + require.NotNil(s.T(), req) + resp, err := http.DefaultClient.Do(req) + if err != nil { + time.Sleep(time.Second) + continue + } + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() + if resp.StatusCode != http.StatusUnauthorized { + // The server may be running but still not fully ready to accept requests + time.Sleep(time.Second) + continue + } + // Server is up and running! + ready = true + break + } + require.True(s.T(), ready, "Proxy is not ready after %d seconds", sec) +} + +func (s *TestProxySuite) checkProxyIsHealthy(port string) { + req, err := http.NewRequest("GET", fmt.Sprintf("http://localhost:%s/proxyhealth", port), nil) + require.NoError(s.T(), err) + require.NotNil(s.T(), req) + + // when + resp, err := http.DefaultClient.Do(req) + + // then + require.NoError(s.T(), err) + require.NotNil(s.T(), resp) + defer resp.Body.Close() + assert.Equal(s.T(), http.StatusOK, resp.StatusCode) + s.assertResponseBody(resp, `{"alive": true}`) +} + func (s *TestProxySuite) checkPlainHTTPErrors(fakeApp *fake.ProxyFakeApp) { s.Run("plain http error", func() { s.Run("unauthorized if no token present", func() { @@ -382,7 +398,7 @@ func (s *TestProxySuite) checkWebLogin() { }) } -func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { +func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy, publicViewerEnabled bool) { s.Run("successfully proxy", func() { userID := uuid.New() @@ -665,6 +681,9 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { if req.Values().List()[0] == "smith2" || req.Values().List()[0] == "mycoolworkspace" { spaceBindings = []toolchainv1alpha1.SpaceBinding{*fake.NewSpaceBinding("mycoolworkspace-smith2", "smith2", "mycoolworkspace", "admin")} } + if publicViewerEnabled && req.Values().List()[0] == toolchainv1alpha1.KubesawAuthenticatedUsername { + spaceBindings = []toolchainv1alpha1.SpaceBinding{*fake.NewSpaceBinding("communityspace-publicviewer", "publicviewer", "communityspace", "viewer")} + } } return spaceBindings, nil } @@ -692,6 +711,7 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { } s.Application.MockInformerService(inf) fakeApp.MemberClusterServiceMock = s.newMemberClusterServiceWithMembers(testServer.URL) + fakeApp.InformerServiceMock = inf p.spaceLister = &handlers.SpaceLister{ GetSignupFunc: fakeApp.SignupServiceMock.GetSignupFromInformer, diff --git a/pkg/proxy/service/cluster_service.go b/pkg/proxy/service/cluster_service.go index 95551105..ca7ca26f 100644 --- a/pkg/proxy/service/cluster_service.go +++ b/pkg/proxy/service/cluster_service.go @@ -40,21 +40,21 @@ func NewMemberClusterService(context servicecontext.ServiceContext, options ...O return si } -func (s *ServiceImpl) GetClusterAccess(userID, username, workspace, proxyPluginName string) (*access.ClusterAccess, error) { - signup, err := s.Services().SignupService().GetSignupFromInformer(nil, userID, username, false) // don't check for usersignup complete status, since it might cause the proxy blocking the request and returning an error when quick transitions from ready to provisioning are happening. - if err != nil { - return nil, err - } - // if signup has the CompliantUsername set it means that MUR was created and useraccount is provisioned - if signup == nil || signup.CompliantUsername == "" { - cause := errs.New("user is not provisioned (yet)") - log.Error(nil, cause, fmt.Sprintf("signup object: %+v", signup)) - return nil, cause - } - +func (s *ServiceImpl) GetClusterAccess(userID, username, workspace, proxyPluginName string, publicViewerEnabled bool) (*access.ClusterAccess, error) { // if workspace is not provided then return the default space access if workspace == "" { - return s.accessForCluster(signup.APIEndpoint, signup.ClusterName, signup.CompliantUsername, proxyPluginName) + return s.getClusterAccessForDefaultWorkspace(userID, username, proxyPluginName) + } + + return s.getSpaceAccess(userID, username, workspace, proxyPluginName, publicViewerEnabled) +} + +// getSpaceAccess retrieves space access for an user +func (s *ServiceImpl) getSpaceAccess(userID, username, workspace, proxyPluginName string, publicViewerEnabled bool) (*access.ClusterAccess, error) { + // retrieve the user's complaint name + userName, err := s.getUserSignupComplaintName(userID, username, publicViewerEnabled) + if err != nil { + return nil, err } // look up space @@ -65,7 +65,49 @@ func (s *ServiceImpl) GetClusterAccess(userID, username, workspace, proxyPluginN return nil, fmt.Errorf("the requested space is not available") } - return s.accessForSpace(space, signup.CompliantUsername, proxyPluginName) + return s.accessForSpace(space, userName, proxyPluginName) +} + +func (s *ServiceImpl) getUserSignupComplaintName(userID, username string, publicViewerEnabled bool) (string, error) { + // if PublicViewer is enabled and the requested user is the PublicViewer, than no lookup is required + if publicViewerEnabled && username == userID && userID == toolchainv1alpha1.KubesawAuthenticatedUsername { + return username, nil + } + + // retrieve the UserSignup from cache + // + // don't check for usersignup complete status, since it might cause the proxy blocking the request + // and returning an error when quick transitions from ready to provisioning are happening. + userSignup, err := s.Services().SignupService().GetSignupFromInformer(nil, userID, username, false) + if err != nil { + return "", err + } + // if signup has the CompliantUsername set it means that MUR was created and useraccount is provisioned + if userSignup == nil || userSignup.CompliantUsername == "" { + cause := errs.New("user is not provisioned (yet)") + log.Error(nil, cause, fmt.Sprintf("signup object: %+v", userSignup)) + return "", cause + } + + return userSignup.CompliantUsername, nil +} + +// getClusterAccessForDefaultWorkspace retrieves the cluster for the user's default workspace +func (s *ServiceImpl) getClusterAccessForDefaultWorkspace(userID, username, proxyPluginName string) (*access.ClusterAccess, error) { + // don't check for usersignup complete status, since it might cause the proxy blocking the request + // and returning an error when quick transitions from ready to provisioning are happening. + signup, err := s.Services().SignupService().GetSignupFromInformer(nil, userID, username, false) + if err != nil { + return nil, err + } + // if signup has the CompliantUsername set it means that MUR was created and useraccount is provisioned + if signup == nil || signup.CompliantUsername == "" { + cause := errs.New("user is not provisioned (yet)") + log.Error(nil, cause, fmt.Sprintf("signup object: %+v", signup)) + return nil, cause + } + + return s.accessForCluster(signup.APIEndpoint, signup.ClusterName, signup.CompliantUsername, proxyPluginName) } func (s *ServiceImpl) accessForSpace(space *toolchainv1alpha1.Space, username, proxyPluginName string) (*access.ClusterAccess, error) { diff --git a/pkg/proxy/service/cluster_service_test.go b/pkg/proxy/service/cluster_service_test.go index 6ec55e78..a32dc217 100644 --- a/pkg/proxy/service/cluster_service_test.go +++ b/pkg/proxy/service/cluster_service_test.go @@ -36,7 +36,15 @@ func TestRunClusterServiceSuite(t *testing.T) { suite.Run(t, &TestClusterServiceSuite{test.UnitTestSuite{}}) } -func (s *TestClusterServiceSuite) TestGetClusterAccess() { +func (s *TestClusterServiceSuite) TestGetClusterAccessCommunityDisabled() { + s.testGetClusterAccessCommon(false) +} + +func (s *TestClusterServiceSuite) TestGetClusterAccessCommunityEnabled() { + s.testGetClusterAccessCommon(true) +} + +func (s *TestClusterServiceSuite) testGetClusterAccessCommon(publicViewerEnabled bool) { // given sc := fake.NewSignupService(fake.Signup("123-noise", &signup.Signup{ CompliantUsername: "noise1", @@ -114,7 +122,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { } // when - _, err := svc.GetClusterAccess("789-ready", "", "", "") + _, err := svc.GetClusterAccess("789-ready", "", "", "", publicViewerEnabled) // then require.EqualError(s.T(), err, "oopsi woopsi") @@ -124,7 +132,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("userid is not found", func() { // when - _, err := svc.GetClusterAccess("unknown_id", "", "", "") + _, err := svc.GetClusterAccess("unknown_id", "", "", "", publicViewerEnabled) // then require.EqualError(s.T(), err, "user is not provisioned (yet)") @@ -132,7 +140,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("username is not found", func() { // when - _, err := svc.GetClusterAccess("", "unknown_username", "", "") + _, err := svc.GetClusterAccess("", "unknown_username", "", "", publicViewerEnabled) // then require.EqualError(s.T(), err, "user is not provisioned (yet)") @@ -140,7 +148,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("user is not provisioned yet", func() { // when - _, err := svc.GetClusterAccess("456-not-ready", "", "", "") + _, err := svc.GetClusterAccess("456-not-ready", "", "", "", publicViewerEnabled) // then require.EqualError(s.T(), err, "user is not provisioned (yet)") @@ -160,7 +168,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Application.MockInformerService(inf) // when - _, err := svc.GetClusterAccess("789-ready", "", "smith2", "") + _, err := svc.GetClusterAccess("789-ready", "", "smith2", "", publicViewerEnabled) // then // original error is only logged so that it doesn't reveal information about a space that may not belong to the requestor @@ -169,7 +177,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("space not found", func() { // when - _, err := svc.GetClusterAccess("789-ready", "", "unknown", "") // unknown workspace requested + _, err := svc.GetClusterAccess("789-ready", "", "unknown", "", publicViewerEnabled) // unknown workspace requested // then require.EqualError(s.T(), err, "the requested space is not available") @@ -191,7 +199,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { ) s.Run("default workspace case", func() { // when - _, err := svc.GetClusterAccess("789-ready", "", "", "") + _, err := svc.GetClusterAccess("789-ready", "", "", "", publicViewerEnabled) // then require.EqualError(s.T(), err, "no member clusters found") @@ -199,7 +207,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("workspace context case", func() { // when - _, err := svc.GetClusterAccess("789-ready", "", "smith2", "") + _, err := svc.GetClusterAccess("789-ready", "", "smith2", "", publicViewerEnabled) // then require.EqualError(s.T(), err, "no member clusters found") @@ -221,7 +229,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("default workspace case", func() { // when - _, err := svc.GetClusterAccess("012-ready-unknown-cluster", "", "", "") + _, err := svc.GetClusterAccess("012-ready-unknown-cluster", "", "", "", publicViewerEnabled) // then require.EqualError(s.T(), err, "no member cluster found for the user") @@ -229,7 +237,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("workspace context case", func() { // when - _, err := svc.GetClusterAccess("012-ready-unknown-cluster", "", "unknown-cluster", "") + _, err := svc.GetClusterAccess("012-ready-unknown-cluster", "", "unknown-cluster", "", publicViewerEnabled) // then require.EqualError(s.T(), err, "no member cluster found for space 'unknown-cluster'") @@ -299,7 +307,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { expectedToken := "abc123" // should match member 2 bearer token // when - ca, err := svc.GetClusterAccess("789-ready", "", "", "tekton-results") + ca, err := svc.GetClusterAccess("789-ready", "", "", "tekton-results", publicViewerEnabled) // then require.NoError(s.T(), err) @@ -312,7 +320,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("cluster access correct when username provided", func() { // when - ca, err := svc.GetClusterAccess("", "smith@", "", "tekton-results") + ca, err := svc.GetClusterAccess("", "smith@", "", "tekton-results", publicViewerEnabled) // then require.NoError(s.T(), err) @@ -325,7 +333,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("cluster access correct when using workspace context", func() { // when - ca, err := svc.GetClusterAccess("789-ready", "", "smith2", "tekton-results") // workspace-context specified + ca, err := svc.GetClusterAccess("789-ready", "", "smith2", "tekton-results", publicViewerEnabled) // workspace-context specified // then require.NoError(s.T(), err) @@ -353,7 +361,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { return memberClient.Client.Get(ctx, key, obj, opts...) } memberArray[0].Client = mC - ca, err := svc.GetClusterAccess("789-ready", "", "teamspace", "tekton-results") // workspace-context specified + ca, err := svc.GetClusterAccess("789-ready", "", "teamspace", "tekton-results", publicViewerEnabled) // workspace-context specified // then require.NoError(s.T(), err) @@ -371,7 +379,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { expectedToken := "abc123" // should match member 2 bearer token // when - ca, err := svc.GetClusterAccess("789-ready", "", "", "") + ca, err := svc.GetClusterAccess("789-ready", "", "", "", publicViewerEnabled) // then require.NoError(s.T(), err) @@ -384,7 +392,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("cluster access correct when username provided", func() { // when - ca, err := svc.GetClusterAccess("", "smith@", "", "") + ca, err := svc.GetClusterAccess("", "smith@", "", "", publicViewerEnabled) // then require.NoError(s.T(), err) @@ -397,7 +405,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("cluster access correct when using workspace context", func() { // when - ca, err := svc.GetClusterAccess("789-ready", "", "smith2", "") // workspace-context specified + ca, err := svc.GetClusterAccess("789-ready", "", "smith2", "", publicViewerEnabled) // workspace-context specified // then require.NoError(s.T(), err) @@ -409,7 +417,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("another workspace on another cluster", func() { // when - ca, err := svc.GetClusterAccess("789-ready", "", "teamspace", "") // workspace-context specified + ca, err := svc.GetClusterAccess("789-ready", "", "teamspace", "", publicViewerEnabled) // workspace-context specified // then require.NoError(s.T(), err) diff --git a/pkg/server/in_cluster_application.go b/pkg/server/in_cluster_application.go index 979cba2d..ad11922b 100644 --- a/pkg/server/in_cluster_application.go +++ b/pkg/server/in_cluster_application.go @@ -29,7 +29,8 @@ func NewInClusterApplication(informer informers.Informer) (application.Applicati serviceFactory: factory.NewServiceFactory( factory.WithServiceContextOptions(factory.CRTClientOption(kubeClient), factory.InformerOption(informer), - )), + ), + ), }, nil } diff --git a/test/fake/proxy.go b/test/fake/proxy.go index bad15e29..22d350cd 100644 --- a/test/fake/proxy.go +++ b/test/fake/proxy.go @@ -17,10 +17,15 @@ type ProxyFakeApp struct { Err error SignupServiceMock service.SignupService MemberClusterServiceMock service.MemberClusterService + InformerServiceMock service.InformerService } func (a *ProxyFakeApp) InformerService() service.InformerService { - panic("InformerService shouldn't be called") + if a.InformerServiceMock == nil { + panic("InformerService shouldn't be called") + } + + return a.InformerServiceMock } func (a *ProxyFakeApp) SignupService() service.SignupService { @@ -45,7 +50,7 @@ type fakeClusterService struct { fakeApp *ProxyFakeApp } -func (f *fakeClusterService) GetClusterAccess(userID, _, _, _ string) (*access.ClusterAccess, error) { +func (f *fakeClusterService) GetClusterAccess(userID, _, _, _ string, _ bool) (*access.ClusterAccess, error) { return f.fakeApp.Accesses[userID], f.fakeApp.Err } From 52156fe11014ade4b72de4277f353bb41cc6db55 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 9 Jul 2024 19:49:16 +0200 Subject: [PATCH 03/70] enable public-viewer support in spacelister_get Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/proxy/handlers/spacelister_get.go b/pkg/proxy/handlers/spacelister_get.go index 6925fce3..dbae1f75 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/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,6 +29,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 From 573aeccaa1e3532f0456f28ee96c74e5a7776f58 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Fri, 12 Jul 2024 17:18:24 +0200 Subject: [PATCH 04/70] add unit tests for pkg/configuration Signed-off-by: Francesco Ilario --- pkg/configuration/configuration_test.go | 39 +++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/pkg/configuration/configuration_test.go b/pkg/configuration/configuration_test.go index 226e0004..34a09104 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) + regServiceCfg := configuration.NewRegistrationServiceConfig(cfg, secrets) + + // then + + // when + assert.Equal(t, tc.expectedValue, regServiceCfg.PublicViewerEnabled()) + }) + } +} From fc99e4e89e24a9caa6d0218c55e2622ac0b1b605 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Fri, 12 Jul 2024 17:41:23 +0200 Subject: [PATCH 05/70] add tests for log Signed-off-by: Francesco Ilario --- pkg/log/log_test.go | 79 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 61 insertions(+), 18 deletions(-) diff --git a/pkg/log/log_test.go b/pkg/log/log_test.go index e251f495..9183df50 100644 --- a/pkg/log/log_test.go +++ b/pkg/log/log_test.go @@ -60,25 +60,68 @@ func TestLog(t *testing.T) { }) t.Run("log infoEchof", func(t *testing.T) { - buf.Reset() - req := httptest.NewRequest(http.MethodGet, "https://api-server.com/api/workspaces/path", strings.NewReader("{}")) - rec := httptest.NewRecorder() - ctx := echo.New().NewContext(req, rec) - ctx.Set(context.SubKey, "test") - ctx.Set(context.UsernameKey, "usernametest") - ctx.Set(context.WorkspaceKey, "coolworkspace") + tt := map[string]struct { + name string + contains string + notContains string + ctxSet map[string]interface{} + }{ + "default": {}, + "impersonateUser is set": { + ctxSet: map[string]interface{}{context.ImpersonateUser: "user"}, + contains: `"impersonate-user":"user"`, + }, + "impersonateUser is not set": { + ctxSet: map[string]interface{}{}, + notContains: `impersonate-user`, + }, + "public-viewer-enabled is set to true": { + ctxSet: map[string]interface{}{context.PublicViewerEnabled: true}, + contains: `"public-viewer-enabled":true`, + }, + "public-viewer-enabled is set to false": { + ctxSet: map[string]interface{}{context.PublicViewerEnabled: false}, + contains: `"public-viewer-enabled":false`, + }, + "public-viewer-enabled is not set": { + ctxSet: map[string]interface{}{}, + notContains: `public-viewer-enabled`, + }, + } - InfoEchof(ctx, "test %s", "info") - value := buf.String() - assert.Contains(t, value, `"logger":"logger_tests"`) - assert.Contains(t, value, `"msg":"test info"`) - assert.Contains(t, value, `"user_id":"test"`) // subject -> user_id - assert.Contains(t, value, `"username":"usernametest"`) - assert.Contains(t, value, `"level":"info"`) - assert.Contains(t, value, `"timestamp":"`) - assert.Contains(t, value, `"workspace":"coolworkspace"`) - assert.Contains(t, value, `"method":"GET"`) - assert.Contains(t, value, `"url":"https://api-server.com/api/workspaces/path"`) + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + buf.Reset() + req := httptest.NewRequest(http.MethodGet, "https://api-server.com/api/workspaces/path", strings.NewReader("{}")) + rec := httptest.NewRecorder() + ctx := echo.New().NewContext(req, rec) + ctx.Set(context.SubKey, "test") + ctx.Set(context.UsernameKey, "usernametest") + ctx.Set(context.WorkspaceKey, "coolworkspace") + for k, v := range tc.ctxSet { + ctx.Set(k, v) + } + + InfoEchof(ctx, "test %s", "info") + value := buf.String() + assert.Contains(t, value, `"logger":"logger_tests"`) + assert.Contains(t, value, `"msg":"test info"`) + assert.Contains(t, value, `"user_id":"test"`) // subject -> user_id + assert.Contains(t, value, `"username":"usernametest"`) + assert.Contains(t, value, `"level":"info"`) + assert.Contains(t, value, `"timestamp":"`) + assert.Contains(t, value, `"workspace":"coolworkspace"`) + assert.Contains(t, value, `"method":"GET"`) + assert.Contains(t, value, `"url":"https://api-server.com/api/workspaces/path"`) + + if tc.contains != "" { + assert.Contains(t, value, tc.contains) + } + if tc.notContains != "" { + assert.NotContains(t, value, tc.notContains) + } + }) + } }) t.Run("log infof with no arguments", func(t *testing.T) { From 962c87ca4e8891011e5067fc00bb9e7ededfc1c7 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Fri, 12 Jul 2024 18:20:47 +0200 Subject: [PATCH 06/70] add unit tests for proxy Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_test.go | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index b530b710..6b90d666 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -27,6 +27,7 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/signup" "github.com/codeready-toolchain/registration-service/test" "github.com/codeready-toolchain/registration-service/test/fake" + "github.com/gin-gonic/gin" "github.com/google/uuid" "github.com/prometheus/client_golang/prometheus" @@ -423,6 +424,9 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy, publ ExpectedProxyResponseHeaders http.Header ExpectedProxyResponseStatus int Standalone bool // If true then the request is not expected to be forwarded to the kube api server + + OverrideGetSignupFunc func(ctx *gin.Context, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error) + ExpectedResponse *string }{ "plain http cors preflight request with no request method": { ProxyRequestMethod: "OPTIONS", @@ -569,6 +573,19 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy, publ }, ExpectedProxyResponseStatus: http.StatusOK, }, + "error retrieving user workspaces": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(userID)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + }, + ExpectedProxyResponseStatus: http.StatusInternalServerError, + // ExpectedResponse: "invalid workspace request: access to workspace 'alice-private' is forbidden", + OverrideGetSignupFunc: func(ctx *gin.Context, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error) { + return nil, fmt.Errorf("test error") + }, + ExpectedResponse: ptr("unable to retrieve user workspaces: test error"), + }, } rejectedHeaders := []headerToAdd{ @@ -720,6 +737,9 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy, publ }, ProxyMetrics: p.metrics, } + if tc.OverrideGetSignupFunc != nil { + p.spaceLister.GetSignupFunc = tc.OverrideGetSignupFunc + } } // when @@ -731,7 +751,9 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy, publ require.NotNil(s.T(), resp) defer resp.Body.Close() assert.Equal(s.T(), tc.ExpectedProxyResponseStatus, resp.StatusCode) - if !tc.Standalone { + if tc.ExpectedResponse != nil { + s.assertResponseBody(resp, *tc.ExpectedResponse) + } else if !tc.Standalone { s.assertResponseBody(resp, "my response") } for hk, hv := range tc.ExpectedProxyResponseHeaders { @@ -817,6 +839,10 @@ var noCORSHeaders = map[string][]string{ "Vary": {}, } +func ptr[T any](t T) *T { + return &t +} + func upgradeToWebsocket(req *http.Request) { req.Header.Set("Connection", "upgrade") req.Header.Set("Upgrade", "websocket") From ec8b47057615586e7f3b52b34031e29064ea90f3 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 15 Jul 2024 10:33:24 +0200 Subject: [PATCH 07/70] Apply suggestions from code review Co-authored-by: Alexey Kazakov --- pkg/context/keys.go | 2 +- pkg/proxy/handlers/spacelister_get.go | 4 +++- pkg/proxy/handlers/spacelister_list.go | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/pkg/context/keys.go b/pkg/context/keys.go index 4ae47251..a6eba535 100644 --- a/pkg/context/keys.go +++ b/pkg/context/keys.go @@ -27,6 +27,6 @@ const ( RequestReceivedTime = "requestReceivedTime" // PublicViewerEnabled is a boolean value indicating whether PublicViewer support is enabled PublicViewerEnabled = "publicViewerEnabled" - // ImpersonateUser is the content key for the impersonated user in proxied call + // ImpersonateUser is the context key for the impersonated user in proxied call ImpersonateUser = "impersonateUser" ) diff --git a/pkg/proxy/handlers/spacelister_get.go b/pkg/proxy/handlers/spacelister_get.go index dbae1f75..2fb1d839 100644 --- a/pkg/proxy/handlers/spacelister_get.go +++ b/pkg/proxy/handlers/spacelister_get.go @@ -107,6 +107,8 @@ 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, userSignup *signup.Signup) (*toolchainv1alpha1.SpaceBinding, error) { // recursively get all the spacebindings for the current workspace listSpaceBindingsFunc := func(spaceName string) ([]toolchainv1alpha1.SpaceBinding, error) { @@ -127,7 +129,7 @@ func getUserSpaceBinding(ctx echo.Context, spaceLister *SpaceLister, space *tool return nil, err } if len(userSpaceBindings) == 0 { - // let's only log the issue and consider this as not found + // consider this as not found return nil, nil } diff --git a/pkg/proxy/handlers/spacelister_list.go b/pkg/proxy/handlers/spacelister_list.go index 3199c4d1..bf170f18 100644 --- a/pkg/proxy/handlers/spacelister_list.go +++ b/pkg/proxy/handlers/spacelister_list.go @@ -61,8 +61,8 @@ func ListUserWorkspaces(ctx echo.Context, spaceLister *SpaceLister) ([]toolchain return workspacesFromSpaceBindings(ctx, spaceLister, signup.Name, spaceBindings), nil } -// getMURNamesForList returns a list MasterUserRecord names to use in listing Workspaces. -// If PublicViewer is enabled, the list will contain at least the PublicViewer username. +// 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 != "" { From 91ff1aa792f6add81d9b22c4546a4d1277da5b18 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 15 Jul 2024 10:36:46 +0200 Subject: [PATCH 08/70] fix linter Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index 6b90d666..d0ab79a5 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -581,7 +581,7 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy, publ }, ExpectedProxyResponseStatus: http.StatusInternalServerError, // ExpectedResponse: "invalid workspace request: access to workspace 'alice-private' is forbidden", - OverrideGetSignupFunc: func(ctx *gin.Context, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error) { + OverrideGetSignupFunc: func(_ *gin.Context, _, _ string, _ bool) (*signup.Signup, error) { return nil, fmt.Errorf("test error") }, ExpectedResponse: ptr("unable to retrieve user workspaces: test error"), From 6c2a59502cc1ead0190a21b7c38892f74bb34a56 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 15 Jul 2024 10:51:27 +0200 Subject: [PATCH 09/70] return empty slice instead of nil Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_list.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_list.go b/pkg/proxy/handlers/spacelister_list.go index bf170f18..8f7035e7 100644 --- a/pkg/proxy/handlers/spacelister_list.go +++ b/pkg/proxy/handlers/spacelister_list.go @@ -42,13 +42,13 @@ func ListUserWorkspaces(ctx echo.Context, spaceLister *SpaceLister) ([]toolchain } // signup is not ready if signup == nil { - return nil, nil + return []toolchainv1alpha1.Workspace{}, nil } // get MUR Names murNames := getMURNamesForList(ctx, signup) if len(murNames) == 0 { - return nil, nil + return []toolchainv1alpha1.Workspace{}, nil } // get all spacebindings with given mur since no workspace was provided From ca4b882d877c6f79995034980342bb92419505b8 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 15 Jul 2024 10:52:00 +0200 Subject: [PATCH 10/70] simplify getUserSpaceBinding Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get.go | 16 ++++++---------- pkg/proxy/handlers/spacelister_get_test.go | 2 +- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get.go b/pkg/proxy/handlers/spacelister_get.go index 2fb1d839..b88603ea 100644 --- a/pkg/proxy/handlers/spacelister_get.go +++ b/pkg/proxy/handlers/spacelister_get.go @@ -76,7 +76,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) + userSpaceBinding, err := getUserSpaceBinding(ctx, spaceLister, space, userSignup.CompliantUsername) if err != nil { return nil, err } @@ -85,13 +85,9 @@ func getUserOrPublicViewerSpaceBinding(ctx echo.Context, spaceLister *SpaceListe // retry with PublicViewer's signup if userSpaceBinding == nil { if context.IsPublicViewerEnabled(ctx) { - publicViewerUserSignup := signup.Signup{ - Name: toolchainv1alpha1.KubesawAuthenticatedUsername, - CompliantUsername: toolchainv1alpha1.KubesawAuthenticatedUsername, - } - pvSb, err := getUserSpaceBinding(ctx, spaceLister, space, &publicViewerUserSignup) + 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", publicViewerUserSignup.CompliantUsername, workspaceName)) + 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 { @@ -109,14 +105,14 @@ 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, userSignup *signup.Signup) (*toolchainv1alpha1.SpaceBinding, error) { +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, err := labels.NewRequirement(toolchainv1alpha1.SpaceBindingSpaceLabelKey, selection.Equals, []string{spaceName}) if err != nil { return nil, err } - murSelector, err := labels.NewRequirement(toolchainv1alpha1.SpaceBindingMasterUserRecordLabelKey, selection.Equals, []string{userSignup.CompliantUsername}) + murSelector, err := labels.NewRequirement(toolchainv1alpha1.SpaceBindingMasterUserRecordLabelKey, selection.Equals, []string{compliantUsername}) if err != nil { return nil, err } @@ -134,7 +130,7 @@ 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", 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 } diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index dfbb7981..45ef9fa0 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -776,7 +776,7 @@ func TestSpaceListerGetCommunityEnabled(t *testing.T) { workspaceRequest: "batman", expectedWorkspace: &batmanWS, }, - "batman can get robin workspace": { + "batman can get robin workspace as public-viewer": { username: "batman.space", workspaceRequest: "robin", expectedWorkspace: func() *toolchainv1alpha1.Workspace { From 7f015b5b2546b702aedadac951a4870ba64222c5 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 15 Jul 2024 10:59:01 +0200 Subject: [PATCH 11/70] cleanup tests Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get_test.go | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index 45ef9fa0..63b1d02c 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -759,12 +759,10 @@ func TestSpaceListerGetCommunityEnabled(t *testing.T) { batmanWS := workspaceFor(t, fakeClient, "batman", "admin", true) tests := map[string]struct { - username string - expectedErr string - workspaceRequest string - expectedWorkspace *toolchainv1alpha1.Workspace - overrideInformerFunc func() service.InformerService - overrideSignupFunc func(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error) + username string + expectedErr string + workspaceRequest string + expectedWorkspace *toolchainv1alpha1.Workspace }{ "robin can get robin workspace": { username: "robin.space", @@ -799,13 +797,7 @@ func TestSpaceListerGetCommunityEnabled(t *testing.T) { t.Run(k, func(t *testing.T) { // given signupProvider := fakeSignupService.GetSignupFromInformer - if tc.overrideSignupFunc != nil { - signupProvider = tc.overrideSignupFunc - } informerFunc := fake.GetInformerService(fakeClient) - if tc.overrideInformerFunc != nil { - informerFunc = tc.overrideInformerFunc - } proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) s := &handlers.SpaceLister{ From 941e0520b6d4cd099e1a2e4c0cd4a1c9737274d9 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 15 Jul 2024 11:03:31 +0200 Subject: [PATCH 12/70] refactor spacelister_get tests Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get_test.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index 63b1d02c..61c1b36c 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -31,12 +31,19 @@ import ( runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" ) -func TestSpaceListerGetCommonCommunityEnabled(t *testing.T) { - testSpaceListerGet(t, true) -} +func TestSpaceListerGet(t *testing.T) { + tt := map[string]struct { + publicViewerEnabled bool + }{ + "public-viewer enabled": {publicViewerEnabled: true}, + "public-viewer disabled": {publicViewerEnabled: false}, + } -func TestSpaceListerGetCommonCommunityDisabled(t *testing.T) { - testSpaceListerGet(t, false) + for k, tc := range tt { + t.Run(k, func(t *testing.T) { + testSpaceListerGet(t, tc.publicViewerEnabled) + }) + } } func testSpaceListerGet(t *testing.T, publicViewerEnabled bool) { From 4981f914f5cee643227155c0cb1711a537903aea Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 15 Jul 2024 14:03:06 +0200 Subject: [PATCH 13/70] refactor 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 61c1b36c..c3ce9438 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -743,7 +743,7 @@ func TestGetUserWorkspace(t *testing.T) { } } -func TestSpaceListerGetCommunityEnabled(t *testing.T) { +func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { fakeSignupService := fake.NewSignupService( newSignup("batman", "batman.space", true), From 76e43ebea86a2f9f1e3dd8f203a9424e8df65e28 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 15 Jul 2024 14:49:49 +0200 Subject: [PATCH 14/70] fix spacelister list Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_list.go | 1 + pkg/proxy/handlers/spacelister_list_test.go | 367 ++++++++++---------- 2 files changed, 187 insertions(+), 181 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_list.go b/pkg/proxy/handlers/spacelister_list.go index 8f7035e7..0beca1e5 100644 --- a/pkg/proxy/handlers/spacelister_list.go +++ b/pkg/proxy/handlers/spacelister_list.go @@ -23,6 +23,7 @@ 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 diff --git a/pkg/proxy/handlers/spacelister_list_test.go b/pkg/proxy/handlers/spacelister_list_test.go index 0d1f3ce3..5937cb5e 100644 --- a/pkg/proxy/handlers/spacelister_list_test.go +++ b/pkg/proxy/handlers/spacelister_list_test.go @@ -22,206 +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 TestSpaceListerListCommunity(t *testing.T) { - publicViewerEnabled := true - fakeSignupService, fakeClient := buildSpaceListerFakes(t, publicViewerEnabled) +func TestSpaceListerListUserWorkspaces(t *testing.T) { 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 + username string + expectedWs func(*test.FakeClient) []toolchainv1alpha1.Workspace + expectedErr string + expectedWorkspace string + publicViewerEnabled bool }{ - "dancelover lists spaces": { + "dancelover lists spaces with public-viewer enabled": { username: "dance.lover", - expectedWs: []toolchainv1alpha1.Workspace{ - workspaceFor(t, fakeClient, "communityspace", "viewer", false), - workspaceFor(t, fakeClient, "dancelover", "admin", true), - workspaceFor(t, fakeClient, "movielover", "other", false), + 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: "", + expectedErr: "", + publicViewerEnabled: true, }, - } - - t.Run("HandleSpaceListRequest", func(t *testing.T) { - for k, tc := range tests { - t.Run(k, func(t *testing.T) { - // given - signupProvider := fakeSignupService.GetSignupFromInformer - if tc.overrideSignupFunc != nil { - signupProvider = tc.overrideSignupFunc - } - - informerFunc := fake.GetInformerService(fakeClient) - if tc.overrideInformerFunc != nil { - informerFunc = tc.overrideInformerFunc - } - - proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) - - s := &handlers.SpaceLister{ - GetSignupFunc: signupProvider, - GetInformerServiceFunc: informerFunc, - ProxyMetrics: proxyMetrics, + "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), } + }, + expectedErr: "", + publicViewerEnabled: false, + }, + } - 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, 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) - } - } - }) - } - }) + 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) { - publicViewerEnabled := false - fakeSignupService, fakeClient := buildSpaceListerFakes(t, publicViewerEnabled) - - 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{ - workspaceFor(t, fakeClient, "dancelover", "admin", true), - workspaceFor(t, fakeClient, "movielover", "other", false), + 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: "", }, - expectedErr: "", - }, - "movielover lists spaces": { - username: "movie.lover", - expectedWs: []toolchainv1alpha1.Workspace{ - workspaceFor(t, fakeClient, "movielover", "admin", true), + "movielover lists spaces": { + username: "movie.lover", + expectedWs: []toolchainv1alpha1.Workspace{ + workspaceFor(t, fakeClient, "movielover", "admin", true), + }, + expectedErr: "", }, - 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, - }, - "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 + "signup has no compliant username set": { + username: "racing.lover", + expectedWs: []toolchainv1alpha1.Workspace{}, + expectedErr: "", + expectedErrCode: 200, }, - }, - "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") + "space not initialized yet": { + username: "panda.lover", + expectedWs: []toolchainv1alpha1.Workspace{}, + expectedErr: "", + expectedErrCode: 200, }, - }, - } - - for k, tc := range tests { - t.Run(k, func(t *testing.T) { - // given - signupProvider := fakeSignupService.GetSignupFromInformer - if tc.overrideSignupFunc != nil { - signupProvider = tc.overrideSignupFunc - } + "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") + }, + }, + } + + for k, tc := range tests { + t.Run(k, func(t *testing.T) { + // given + signupProvider := fakeSignupService.GetSignupFromInformer + if tc.overrideSignupFunc != nil { + signupProvider = tc.overrideSignupFunc + } - informerFunc := fake.GetInformerService(fakeClient) - if tc.overrideInformerFunc != nil { - informerFunc = tc.overrideInformerFunc - } + informerFunc := fake.GetInformerService(fakeClient) + if tc.overrideInformerFunc != nil { + informerFunc = tc.overrideInformerFunc + } - proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) + proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) - s := &handlers.SpaceLister{ - GetSignupFunc: signupProvider, - GetInformerServiceFunc: informerFunc, - ProxyMetrics: proxyMetrics, - } + 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, 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) + 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) + } } - } - }) - } - }) + }) + } + }) + } } From a85b1045e8dac0d763ddaac35e9c527651cc3803 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 15 Jul 2024 17:47:24 +0200 Subject: [PATCH 15/70] enhance spacelister get tests Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get_test.go | 32 ++++------------------ 1 file changed, 6 insertions(+), 26 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index c3ce9438..84663a7f 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -843,7 +843,7 @@ func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { } } -func TestSpaceListerGetWithBindingsCommunityEnabled(t *testing.T) { +func TestSpaceListerGetWithBindingsWithPublicViewerEnabled(t *testing.T) { fakeSignupService := fake.NewSignupService( newSignup("batman", "batman.space", true), @@ -893,12 +893,9 @@ func TestSpaceListerGetWithBindingsCommunityEnabled(t *testing.T) { ) tests := map[string]struct { - username string - expectedErr string - workspaceRequest string - expectedWorkspace *toolchainv1alpha1.Workspace - overrideInformerFunc func() service.InformerService - overrideSignupFunc func(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error) + username string + workspaceRequest string + expectedWorkspace *toolchainv1alpha1.Workspace }{ "robin can get robin workspace": { username: "robin.space", @@ -924,7 +921,6 @@ func TestSpaceListerGetWithBindingsCommunityEnabled(t *testing.T) { username: "robin.space", workspaceRequest: "batman", expectedWorkspace: nil, - expectedErr: "", }, } @@ -933,13 +929,7 @@ func TestSpaceListerGetWithBindingsCommunityEnabled(t *testing.T) { t.Run(k, func(t *testing.T) { // given signupProvider := fakeSignupService.GetSignupFromInformer - if tc.overrideSignupFunc != nil { - signupProvider = tc.overrideSignupFunc - } informerFunc := fake.GetInformerService(fakeClient) - if tc.overrideInformerFunc != nil { - informerFunc = tc.overrideInformerFunc - } proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) s := &handlers.SpaceLister{ @@ -973,18 +963,8 @@ func TestSpaceListerGetWithBindingsCommunityEnabled(t *testing.T) { wrk, err := handlers.GetUserWorkspaceWithBindings(ctx, s, tc.workspaceRequest, getMembersFuncMock) // 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 - } + require.NoError(t, err) + require.Equal(t, tc.expectedWorkspace, wrk) }) } } From 0d3bfc5a2f8e05978a87425ba01ebfefad8cb97f Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 16 Jul 2024 15:16:27 +0200 Subject: [PATCH 16/70] remove unreachable check Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_list.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_list.go b/pkg/proxy/handlers/spacelister_list.go index 0beca1e5..b0328d0d 100644 --- a/pkg/proxy/handlers/spacelister_list.go +++ b/pkg/proxy/handlers/spacelister_list.go @@ -48,9 +48,6 @@ func ListUserWorkspaces(ctx echo.Context, spaceLister *SpaceLister) ([]toolchain // get MUR Names murNames := getMURNamesForList(ctx, signup) - if len(murNames) == 0 { - return []toolchainv1alpha1.Workspace{}, nil - } // get all spacebindings with given mur since no workspace was provided spaceBindings, err := listSpaceBindingsForUsers(spaceLister, murNames) From 9136bb07a0ffec2e9f6be9c91f800c95a178efd0 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 22 Jul 2024 13:29:04 +0200 Subject: [PATCH 17/70] add tests for cluster-service Signed-off-by: Francesco Ilario --- pkg/proxy/service/cluster_service_test.go | 567 ++++++++++++---------- 1 file changed, 307 insertions(+), 260 deletions(-) diff --git a/pkg/proxy/service/cluster_service_test.go b/pkg/proxy/service/cluster_service_test.go index a32dc217..39ff8473 100644 --- a/pkg/proxy/service/cluster_service_test.go +++ b/pkg/proxy/service/cluster_service_test.go @@ -36,15 +36,7 @@ func TestRunClusterServiceSuite(t *testing.T) { suite.Run(t, &TestClusterServiceSuite{test.UnitTestSuite{}}) } -func (s *TestClusterServiceSuite) TestGetClusterAccessCommunityDisabled() { - s.testGetClusterAccessCommon(false) -} - -func (s *TestClusterServiceSuite) TestGetClusterAccessCommunityEnabled() { - s.testGetClusterAccessCommon(true) -} - -func (s *TestClusterServiceSuite) testGetClusterAccessCommon(publicViewerEnabled bool) { +func (s *TestClusterServiceSuite) TestGetClusterAccess() { // given sc := fake.NewSignupService(fake.Signup("123-noise", &signup.Signup{ CompliantUsername: "noise1", @@ -115,320 +107,372 @@ func (s *TestClusterServiceSuite) testGetClusterAccessCommon(publicViewerEnabled }, ) - s.Run("unable to get signup", func() { - s.Run("signup service returns error", func() { - sc.MockGetSignup = func(_, _ string) (*signup.Signup, error) { - return nil, errors.New("oopsi woopsi") - } + tt := map[string]struct { + publicViewerEnabled bool + }{ + "public-viewer enabled": {publicViewerEnabled: true}, + "public-viewer disabled": {publicViewerEnabled: false}, + } - // when - _, err := svc.GetClusterAccess("789-ready", "", "", "", publicViewerEnabled) + for k, tc := range tt { + publicViewerEnabled := tc.publicViewerEnabled - // then - require.EqualError(s.T(), err, "oopsi woopsi") - }) + s.Run(k, func() { - sc.MockGetSignup = sc.DefaultMockGetSignup() // restore the default signup service, so it doesn't return an error anymore + s.Run("unable to get signup", func() { + tt := map[string]struct { + workspace string + }{ + "default workspace": {workspace: ""}, + "not-existing workspace": {workspace: "not-existing"}, + } + for k, tc := range tt { + s.Run(k, func() { + s.Run("signup service returns error", func() { + sc.MockGetSignup = func(_, _ string) (*signup.Signup, error) { + return nil, errors.New("oopsi woopsi") + } - s.Run("userid is not found", func() { - // when - _, err := svc.GetClusterAccess("unknown_id", "", "", "", publicViewerEnabled) + // when + _, err := svc.GetClusterAccess("789-ready", "", tc.workspace, "", publicViewerEnabled) - // then - require.EqualError(s.T(), err, "user is not provisioned (yet)") - }) + // then + require.EqualError(s.T(), err, "oopsi woopsi") + }) - s.Run("username is not found", func() { - // when - _, err := svc.GetClusterAccess("", "unknown_username", "", "", publicViewerEnabled) + sc.MockGetSignup = sc.DefaultMockGetSignup() // restore the default signup service, so it doesn't return an error anymore - // then - require.EqualError(s.T(), err, "user is not provisioned (yet)") - }) + s.Run("userid is not found", func() { + // when + _, err := svc.GetClusterAccess("unknown_id", "", tc.workspace, "", publicViewerEnabled) - s.Run("user is not provisioned yet", func() { - // when - _, err := svc.GetClusterAccess("456-not-ready", "", "", "", publicViewerEnabled) + // then + require.EqualError(s.T(), err, "user is not provisioned (yet)") + }) - // then - require.EqualError(s.T(), err, "user is not provisioned (yet)") - }) - }) - - s.Run("unable to get space", func() { - s.Run("informer service returns error", func() { - original := inf.GetSpaceFunc - defer func() { // restore original GetSpaceFunc after test - inf.GetSpaceFunc = original - s.Application.MockInformerService(inf) - }() - inf.GetSpaceFunc = func(_ string) (*toolchainv1alpha1.Space, error) { // informer error - return nil, fmt.Errorf("oopsi woopsi") - } - s.Application.MockInformerService(inf) + s.Run("username is not found", func() { + // when + _, err := svc.GetClusterAccess("", "unknown_username", tc.workspace, "", publicViewerEnabled) - // when - _, err := svc.GetClusterAccess("789-ready", "", "smith2", "", publicViewerEnabled) - - // then - // original error is only logged so that it doesn't reveal information about a space that may not belong to the requestor - require.EqualError(s.T(), err, "the requested space is not available") - }) + // then + require.EqualError(s.T(), err, "user is not provisioned (yet)") + }) - s.Run("space not found", func() { - // when - _, err := svc.GetClusterAccess("789-ready", "", "unknown", "", publicViewerEnabled) // unknown workspace requested + s.Run("user is not provisioned yet", func() { + // when + _, err := svc.GetClusterAccess("456-not-ready", "", tc.workspace, "", publicViewerEnabled) - // then - require.EqualError(s.T(), err, "the requested space is not available") - }) - }) + // then + require.EqualError(s.T(), err, "user is not provisioned (yet)") + }) - s.Run("no member cluster found", func() { - s.Run("no member clusters", func() { - svc := service.NewMemberClusterService( - fake.MemberClusterServiceContext{ - Client: s, - Svcs: s.Application, - }, - func(si *service.ServiceImpl) { - si.GetMembersFunc = func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { - return []*commoncluster.CachedToolchainCluster{} - } - }, - ) - s.Run("default workspace case", func() { - // when - _, err := svc.GetClusterAccess("789-ready", "", "", "", publicViewerEnabled) - - // then - require.EqualError(s.T(), err, "no member clusters found") - }) - - s.Run("workspace context case", func() { - // when - _, err := svc.GetClusterAccess("789-ready", "", "smith2", "", publicViewerEnabled) - - // then - require.EqualError(s.T(), err, "no member clusters found") + }) + } }) - }) - s.Run("no member cluster with the given URL", func() { - svc := service.NewMemberClusterService( - fake.MemberClusterServiceContext{ - Client: s, - Svcs: s.Application, - }, - func(si *service.ServiceImpl) { - si.GetMembersFunc = func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { - return s.memberClusters() + s.Run("unable to get space", func() { + s.Run("informer service returns error", func() { + original := inf.GetSpaceFunc + defer func() { // restore original GetSpaceFunc after test + inf.GetSpaceFunc = original + s.Application.MockInformerService(inf) + }() + inf.GetSpaceFunc = func(_ string) (*toolchainv1alpha1.Space, error) { // informer error + return nil, fmt.Errorf("oopsi woopsi") } - }, - ) + s.Application.MockInformerService(inf) - s.Run("default workspace case", func() { - // when - _, err := svc.GetClusterAccess("012-ready-unknown-cluster", "", "", "", publicViewerEnabled) + // when + _, err := svc.GetClusterAccess("789-ready", "", "smith2", "", publicViewerEnabled) - // then - require.EqualError(s.T(), err, "no member cluster found for the user") - }) + // then + // original error is only logged so that it doesn't reveal information about a space that may not belong to the requestor + require.EqualError(s.T(), err, "the requested space is not available") + }) - s.Run("workspace context case", func() { - // when - _, err := svc.GetClusterAccess("012-ready-unknown-cluster", "", "unknown-cluster", "", publicViewerEnabled) + s.Run("space not found", func() { + // when + _, err := svc.GetClusterAccess("789-ready", "", "unknown", "", publicViewerEnabled) // unknown workspace requested - // then - require.EqualError(s.T(), err, "no member cluster found for space 'unknown-cluster'") + // then + require.EqualError(s.T(), err, "the requested space is not available") + }) }) - }) - }) - s.Run("member found", func() { - memberClient := commontest.NewFakeClient(s.T()) - memberArray := []*commoncluster.CachedToolchainCluster{ - { - Config: &commoncluster.Config{ - Name: "member-1", - APIEndpoint: "https://api.endpoint.member-1.com:6443", - RestConfig: &rest.Config{ - BearerToken: "def456", - }, - }, - }, - { - Config: &commoncluster.Config{ - Name: "member-2", - APIEndpoint: "https://api.endpoint.member-2.com:6443", - OperatorNamespace: "member-operator", - RestConfig: &rest.Config{ - BearerToken: "abc123", - }, - }, - Client: memberClient, - }, - { - Config: &commoncluster.Config{ - Name: "member-3", - APIEndpoint: "https://api.endpoint.member-3.com:6443", - RestConfig: &rest.Config{}, - }, - }, - } - - svc := service.NewMemberClusterService( - fake.MemberClusterServiceContext{ - Client: s, - Svcs: s.Application, - }, - func(si *service.ServiceImpl) { - si.GetMembersFunc = func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { - return memberArray - } - }, - ) - - s.Run("verify cluster access with route", func() { - memberClient.MockGet = func(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { - route, ok := obj.(*routev1.Route) - if ok && key.Namespace == "tekton-results" && key.Name == "tekton-results" { - route.Namespace = key.Namespace - route.Name = key.Name - route.Status.Ingress = []routev1.RouteIngress{ - { - Host: "myservice.endpoint.member-2.com", + s.Run("no member cluster found", func() { + s.Run("no member clusters", func() { + svc := service.NewMemberClusterService( + fake.MemberClusterServiceContext{ + Client: s, + Svcs: s.Application, }, - } - return nil - } - return memberClient.Client.Get(ctx, key, obj, opts...) - } - expectedToken := "abc123" // should match member 2 bearer token + func(si *service.ServiceImpl) { + si.GetMembersFunc = func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { + return []*commoncluster.CachedToolchainCluster{} + } + }, + ) + s.Run("default workspace case", func() { + // when + _, err := svc.GetClusterAccess("789-ready", "", "", "", publicViewerEnabled) + + // then + require.EqualError(s.T(), err, "no member clusters found") + }) + + s.Run("workspace context case", func() { + // when + _, err := svc.GetClusterAccess("789-ready", "", "smith2", "", publicViewerEnabled) + + // then + require.EqualError(s.T(), err, "no member clusters found") + }) + }) - // when - ca, err := svc.GetClusterAccess("789-ready", "", "", "tekton-results", publicViewerEnabled) + s.Run("no member cluster with the given URL", func() { + svc := service.NewMemberClusterService( + fake.MemberClusterServiceContext{ + Client: s, + Svcs: s.Application, + }, + func(si *service.ServiceImpl) { + si.GetMembersFunc = func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { + return s.memberClusters() + } + }, + ) - // then - require.NoError(s.T(), err) - require.NotNil(s.T(), ca) - expectedURL, err := url.Parse("https://myservice.endpoint.member-2.com") - require.NoError(s.T(), err) - assert.Equal(s.T(), "smith2", ca.Username()) + s.Run("default workspace case", func() { + // when + _, err := svc.GetClusterAccess("012-ready-unknown-cluster", "", "", "", publicViewerEnabled) - s.assertClusterAccess(access.NewClusterAccess(*expectedURL, expectedToken, ""), ca) + // then + require.EqualError(s.T(), err, "no member cluster found for the user") + }) - s.Run("cluster access correct when username provided", func() { - // when - ca, err := svc.GetClusterAccess("", "smith@", "", "tekton-results", publicViewerEnabled) + s.Run("workspace context case", func() { + // when + _, err := svc.GetClusterAccess("012-ready-unknown-cluster", "", "unknown-cluster", "", publicViewerEnabled) - // then - require.NoError(s.T(), err) - require.NotNil(s.T(), ca) - expectedURL, err := url.Parse("https://myservice.endpoint.member-2.com") - require.NoError(s.T(), err) - s.assertClusterAccess(access.NewClusterAccess(*expectedURL, expectedToken, "smith"), ca) - assert.Equal(s.T(), "smith2", ca.Username()) + // then + require.EqualError(s.T(), err, "no member cluster found for space 'unknown-cluster'") + }) + }) }) - s.Run("cluster access correct when using workspace context", func() { - // when - ca, err := svc.GetClusterAccess("789-ready", "", "smith2", "tekton-results", publicViewerEnabled) // workspace-context specified + s.Run("member found", func() { + memberClient := commontest.NewFakeClient(s.T()) + memberArray := []*commoncluster.CachedToolchainCluster{ + { + Config: &commoncluster.Config{ + Name: "member-1", + APIEndpoint: "https://api.endpoint.member-1.com:6443", + RestConfig: &rest.Config{ + BearerToken: "def456", + }, + }, + }, + { + Config: &commoncluster.Config{ + Name: "member-2", + APIEndpoint: "https://api.endpoint.member-2.com:6443", + OperatorNamespace: "member-operator", + RestConfig: &rest.Config{ + BearerToken: "abc123", + }, + }, + Client: memberClient, + }, + { + Config: &commoncluster.Config{ + Name: "member-3", + APIEndpoint: "https://api.endpoint.member-3.com:6443", + RestConfig: &rest.Config{}, + }, + }, + } - // then - require.NoError(s.T(), err) - require.NotNil(s.T(), ca) - expectedURL, err := url.Parse("https://myservice.endpoint.member-2.com") - require.NoError(s.T(), err) - s.assertClusterAccess(access.NewClusterAccess(*expectedURL, expectedToken, "smith"), ca) - assert.Equal(s.T(), "smith2", ca.Username()) + svc := service.NewMemberClusterService( + fake.MemberClusterServiceContext{ + Client: s, + Svcs: s.Application, + }, + func(si *service.ServiceImpl) { + si.GetMembersFunc = func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { + return memberArray + } + }, + ) - s.Run("another workspace on another cluster", func() { - // when - mC := commontest.NewFakeClient(s.T()) - mC.MockGet = func(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + s.Run("verify cluster access with route", func() { + memberClient.MockGet = func(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { route, ok := obj.(*routev1.Route) if ok && key.Namespace == "tekton-results" && key.Name == "tekton-results" { route.Namespace = key.Namespace route.Name = key.Name route.Status.Ingress = []routev1.RouteIngress{ { - Host: "api.endpoint.member-1.com:6443", + Host: "myservice.endpoint.member-2.com", }, } return nil } return memberClient.Client.Get(ctx, key, obj, opts...) } - memberArray[0].Client = mC - ca, err := svc.GetClusterAccess("789-ready", "", "teamspace", "tekton-results", publicViewerEnabled) // workspace-context specified + expectedToken := "abc123" // should match member 2 bearer token + + // when + ca, err := svc.GetClusterAccess("789-ready", "", "", "tekton-results", publicViewerEnabled) // then require.NoError(s.T(), err) require.NotNil(s.T(), ca) - expectedURL, err := url.Parse("https://api.endpoint.member-1.com:6443") + expectedURL, err := url.Parse("https://myservice.endpoint.member-2.com") require.NoError(s.T(), err) - s.assertClusterAccess(access.NewClusterAccess(*expectedURL, "def456", "smith"), ca) assert.Equal(s.T(), "smith2", ca.Username()) - }) - }) - }) - s.Run("verify cluster access no route", func() { - memberClient.MockGet = nil - expectedToken := "abc123" // should match member 2 bearer token - - // when - ca, err := svc.GetClusterAccess("789-ready", "", "", "", publicViewerEnabled) - - // then - require.NoError(s.T(), err) - require.NotNil(s.T(), ca) - expectedURL, err := url.Parse("https://api.endpoint.member-2.com:6443") - require.NoError(s.T(), err) - assert.Equal(s.T(), "smith2", ca.Username()) - - s.assertClusterAccess(access.NewClusterAccess(*expectedURL, expectedToken, ""), ca) - - s.Run("cluster access correct when username provided", func() { - // when - ca, err := svc.GetClusterAccess("", "smith@", "", "", publicViewerEnabled) - - // then - require.NoError(s.T(), err) - require.NotNil(s.T(), ca) - expectedURL, err := url.Parse("https://api.endpoint.member-2.com:6443") - require.NoError(s.T(), err) - s.assertClusterAccess(access.NewClusterAccess(*expectedURL, expectedToken, "smith"), ca) - assert.Equal(s.T(), "smith2", ca.Username()) - }) - - s.Run("cluster access correct when using workspace context", func() { - // when - ca, err := svc.GetClusterAccess("789-ready", "", "smith2", "", publicViewerEnabled) // workspace-context specified + s.assertClusterAccess(access.NewClusterAccess(*expectedURL, expectedToken, ""), ca) + + s.Run("cluster access correct when username provided", func() { + // when + ca, err := svc.GetClusterAccess("", "smith@", "", "tekton-results", publicViewerEnabled) + + // then + require.NoError(s.T(), err) + require.NotNil(s.T(), ca) + expectedURL, err := url.Parse("https://myservice.endpoint.member-2.com") + require.NoError(s.T(), err) + s.assertClusterAccess(access.NewClusterAccess(*expectedURL, expectedToken, "smith"), ca) + assert.Equal(s.T(), "smith2", ca.Username()) + }) + + s.Run("cluster access correct when using workspace context", func() { + // when + ca, err := svc.GetClusterAccess("789-ready", "", "smith2", "tekton-results", publicViewerEnabled) // workspace-context specified + + // then + require.NoError(s.T(), err) + require.NotNil(s.T(), ca) + expectedURL, err := url.Parse("https://myservice.endpoint.member-2.com") + require.NoError(s.T(), err) + s.assertClusterAccess(access.NewClusterAccess(*expectedURL, expectedToken, "smith"), ca) + assert.Equal(s.T(), "smith2", ca.Username()) + + s.Run("another workspace on another cluster", func() { + // when + mC := commontest.NewFakeClient(s.T()) + mC.MockGet = func(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + route, ok := obj.(*routev1.Route) + if ok && key.Namespace == "tekton-results" && key.Name == "tekton-results" { + route.Namespace = key.Namespace + route.Name = key.Name + route.Status.Ingress = []routev1.RouteIngress{ + { + Host: "api.endpoint.member-1.com:6443", + }, + } + return nil + } + return memberClient.Client.Get(ctx, key, obj, opts...) + } + memberArray[0].Client = mC + ca, err := svc.GetClusterAccess("789-ready", "", "teamspace", "tekton-results", publicViewerEnabled) // workspace-context specified + + // then + require.NoError(s.T(), err) + require.NotNil(s.T(), ca) + expectedURL, err := url.Parse("https://api.endpoint.member-1.com:6443") + require.NoError(s.T(), err) + s.assertClusterAccess(access.NewClusterAccess(*expectedURL, "def456", "smith"), ca) + assert.Equal(s.T(), "smith2", ca.Username()) + }) + }) + }) - // then - require.NoError(s.T(), err) - require.NotNil(s.T(), ca) - expectedURL, err := url.Parse("https://api.endpoint.member-2.com:6443") - require.NoError(s.T(), err) - s.assertClusterAccess(access.NewClusterAccess(*expectedURL, expectedToken, "smith"), ca) - assert.Equal(s.T(), "smith2", ca.Username()) + s.Run("verify cluster access no route", func() { + memberClient.MockGet = nil + expectedToken := "abc123" // should match member 2 bearer token - s.Run("another workspace on another cluster", func() { // when - ca, err := svc.GetClusterAccess("789-ready", "", "teamspace", "", publicViewerEnabled) // workspace-context specified + ca, err := svc.GetClusterAccess("789-ready", "", "", "", publicViewerEnabled) // then require.NoError(s.T(), err) require.NotNil(s.T(), ca) - expectedURL, err := url.Parse("https://api.endpoint.member-1.com:6443") + expectedURL, err := url.Parse("https://api.endpoint.member-2.com:6443") require.NoError(s.T(), err) - s.assertClusterAccess(access.NewClusterAccess(*expectedURL, "def456", "smith"), ca) assert.Equal(s.T(), "smith2", ca.Username()) + + s.assertClusterAccess(access.NewClusterAccess(*expectedURL, expectedToken, ""), ca) + + s.Run("cluster access correct when username provided", func() { + // when + ca, err := svc.GetClusterAccess("", "smith@", "", "", publicViewerEnabled) + + // then + require.NoError(s.T(), err) + require.NotNil(s.T(), ca) + expectedURL, err := url.Parse("https://api.endpoint.member-2.com:6443") + require.NoError(s.T(), err) + s.assertClusterAccess(access.NewClusterAccess(*expectedURL, expectedToken, "smith"), ca) + assert.Equal(s.T(), "smith2", ca.Username()) + }) + + s.Run("cluster access correct when using workspace context", func() { + // when + ca, err := svc.GetClusterAccess("789-ready", "", "smith2", "", publicViewerEnabled) // workspace-context specified + + // then + require.NoError(s.T(), err) + require.NotNil(s.T(), ca) + expectedURL, err := url.Parse("https://api.endpoint.member-2.com:6443") + require.NoError(s.T(), err) + s.assertClusterAccess(access.NewClusterAccess(*expectedURL, expectedToken, "smith"), ca) + assert.Equal(s.T(), "smith2", ca.Username()) + + s.Run("another workspace on another cluster", func() { + // when + ca, err := svc.GetClusterAccess("789-ready", "", "teamspace", "", publicViewerEnabled) // workspace-context specified + + // then + require.NoError(s.T(), err) + require.NotNil(s.T(), ca) + expectedURL, err := url.Parse("https://api.endpoint.member-1.com:6443") + require.NoError(s.T(), err) + s.assertClusterAccess(access.NewClusterAccess(*expectedURL, "def456", "smith"), ca) + assert.Equal(s.T(), "smith2", ca.Username()) + }) + }) }) }) }) + } + + // public-viewer enabled + s.Run("user is public-viewer", func() { + s.Run("always get space", func() { + svc := service.NewMemberClusterService( + fake.MemberClusterServiceContext{ + Client: s, + Svcs: s.Application, + }, + func(si *service.ServiceImpl) { + si.GetMembersFunc = func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { + return s.memberClusters() + } + }, + ) + _, err := svc.GetClusterAccess(toolchainv1alpha1.KubesawAuthenticatedUsername, toolchainv1alpha1.KubesawAuthenticatedUsername, "smith2", "", true) + require.NoError(s.T(), err) + }) + + s.Run("has no default workspace", func() { + // when + _, err := svc.GetClusterAccess(toolchainv1alpha1.KubesawAuthenticatedUsername, toolchainv1alpha1.KubesawAuthenticatedUsername, "", "", true) + + // then + require.EqualError(s.T(), err, "user is not provisioned (yet)") + }) }) } @@ -449,6 +493,9 @@ func (s *TestClusterServiceSuite) memberClusters() []*commoncluster.CachedToolch Name: clusterName, APIEndpoint: fmt.Sprintf("https://api.endpoint.%s.com:6443", clusterName), OperatorNamespace: "member-operator", + RestConfig: &rest.Config{ + BearerToken: "token", + }, }, Client: nil, }) From 7ec2cb464cdb1576b480132e1d343896e30a9713 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 22 Jul 2024 17:00:47 +0200 Subject: [PATCH 18/70] add user validation test Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index d0ab79a5..a2b9002b 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -515,6 +515,16 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy, publ }, ExpectedProxyResponseStatus: http.StatusOK, }, + "proxy plain http actual request as not provisioned user": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(uuid.New())}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {"smith3"}, + }, + ExpectedResponse: &[]string{"unable to get target cluster: user is not provisioned (yet)"}[0], + ExpectedProxyResponseStatus: http.StatusInternalServerError, + }, "proxy plain http actual request": { ProxyRequestMethod: "GET", ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(userID)}}, From f362ac5def5878af1864d086478d543000dd974a Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 22 Jul 2024 18:29:46 +0200 Subject: [PATCH 19/70] add tests for validateSpace Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_test.go | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index a2b9002b..c91955b0 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -419,6 +419,7 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy, publ tests := map[string]struct { ProxyRequestMethod string + ProxyRequestPaths map[string]string ProxyRequestHeaders http.Header ExpectedAPIServerRequestHeaders http.Header ExpectedProxyResponseHeaders http.Header @@ -596,6 +597,18 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy, publ }, ExpectedResponse: ptr("unable to retrieve user workspaces: test error"), }, + "unauthorized if workspace not exists": { + ProxyRequestPaths: map[string]string{ + "not-existing-workspace-namespace": "http://localhost:8081/workspaces/not-existing-workspace/api/namespaces/not-existing-namespace/pods", + }, + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(userID)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + }, + ExpectedResponse: ptr("unable to get target cluster: the requested space is not available"), + ExpectedProxyResponseStatus: http.StatusInternalServerError, + }, } rejectedHeaders := []headerToAdd{ @@ -614,6 +627,14 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy, publ for k, tc := range tests { s.Run(k, func() { + paths := tc.ProxyRequestPaths + if len(paths) == 0 { + paths = map[string]string{ + "default workspace": "http://localhost:8081/api/mycoolworkspace/pods", + "workspace context": "http://localhost:8081/workspaces/mycoolworkspace/api/mycoolworkspace/pods", + "proxy plugin context": "http://localhost:8081/plugins/myplugin/workspaces/mycoolworkspace/api/mycoolworkspace/pods", + } + } for _, firstHeader := range rejectedHeaders { rejectedHeadersToAdd := []headerToAdd{firstHeader} @@ -622,11 +643,7 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy, publ // Test each request using both the default workspace URL and a URL that uses the // workspace context. Both should yield the same results. - for workspaceContext, reqPath := range map[string]string{ - "default workspace": "http://localhost:8081/api/mycoolworkspace/pods", - "workspace context": "http://localhost:8081/workspaces/mycoolworkspace/api/mycoolworkspace/pods", - "proxy plugin context": "http://localhost:8081/plugins/myplugin/workspaces/mycoolworkspace/api/mycoolworkspace/pods", - } { + for workspaceContext, reqPath := range paths { s.Run(workspaceContext, func() { // given req, err := http.NewRequest(tc.ProxyRequestMethod, reqPath, nil) From 3b7d1c23e6e05bd5e1c34caeb449e74ad7bcb720 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 22 Jul 2024 18:29:56 +0200 Subject: [PATCH 20/70] use ptr Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index c91955b0..0c5151df 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -523,7 +523,7 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy, publ "Authorization": {"Bearer clusterSAToken"}, "Impersonate-User": {"smith3"}, }, - ExpectedResponse: &[]string{"unable to get target cluster: user is not provisioned (yet)"}[0], + ExpectedResponse: ptr("unable to get target cluster: user is not provisioned (yet)"), ExpectedProxyResponseStatus: http.StatusInternalServerError, }, "proxy plain http actual request": { From 650a6a2e0d85660531dd6ffc1fbca105f7abfe67 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 22 Jul 2024 18:57:21 +0200 Subject: [PATCH 21/70] add test case for non-ready user Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get_test.go | 23 +++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index 84663a7f..086809a5 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -764,7 +764,12 @@ func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { 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 @@ -782,14 +787,9 @@ func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { expectedWorkspace: &batmanWS, }, "batman can get robin workspace as public-viewer": { - username: "batman.space", - workspaceRequest: "robin", - expectedWorkspace: func() *toolchainv1alpha1.Workspace { - batmansRobinWS := robinWS.DeepCopy() - batmansRobinWS.Status.Type = "" - batmansRobinWS.Status.Role = "viewer" - return batmansRobinWS - }(), + username: "batman.space", + workspaceRequest: "robin", + expectedWorkspace: publicRobinWS, }, "robin can not get batman workspace": { username: "robin.space", @@ -797,6 +797,11 @@ func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { expectedWorkspace: nil, expectedErr: "", }, + "gordon can get batman workspace": { + username: "gordon.space", + workspaceRequest: "robin", + expectedWorkspace: publicRobinWS, + }, } for k, tc := range tests { From 4e51460668e680219feaf877de209397ae57665c Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 25 Jul 2024 09:58:45 +0200 Subject: [PATCH 22/70] fix configuration tests comments Signed-off-by: Francesco Ilario --- pkg/configuration/configuration_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/configuration/configuration_test.go b/pkg/configuration/configuration_test.go index 34a09104..812718e9 100644 --- a/pkg/configuration/configuration_test.go +++ b/pkg/configuration/configuration_test.go @@ -183,11 +183,11 @@ func TestPublicViewer(t *testing.T) { cfg := commonconfig.NewToolchainConfigObjWithReset(t) cfg.Spec.Host.PublicViewerConfig = tc.publicViewerConfig secrets := make(map[string]map[string]string) + + // when regServiceCfg := configuration.NewRegistrationServiceConfig(cfg, secrets) // then - - // when assert.Equal(t, tc.expectedValue, regServiceCfg.PublicViewerEnabled()) }) } From dfa1533ef072e0d539e74b01a502d81ce2195ea8 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 25 Jul 2024 10:05:28 +0200 Subject: [PATCH 23/70] Update pkg/proxy/handlers/spacelister_get.go Co-authored-by: Francisc Munteanu --- 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 5bd323ba..7033c60d 100644 --- a/pkg/proxy/handlers/spacelister_get.go +++ b/pkg/proxy/handlers/spacelister_get.go @@ -90,7 +90,7 @@ func getUserOrPublicViewerSpaceBinding(ctx echo.Context, spaceLister *SpaceListe 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", userSignup.CompliantUsername, workspaceName)) + 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 From 69be0bf38664d7c270de801fc02863a741105a70 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 25 Jul 2024 10:43:46 +0200 Subject: [PATCH 24/70] refactor proxy.go Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 32 ++++++++++---------------------- pkg/proxy/proxy_test.go | 2 +- 2 files changed, 11 insertions(+), 23 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 581ff967..0737ce40 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -313,7 +313,7 @@ func (p *Proxy) processHomeWorkspaceRequest(ctx echo.Context, userID, username, // check whether the user's has access to the home workspace // and whether the requestedNamespace -if any- exists in the workspace. requestedNamespace := namespaceFromCtx(ctx) - if err := validateWorkspaceRequest("", requestedNamespace, workspaces); err != nil { + if err := validateWorkspaceRequest("", requestedNamespace, workspaces...); err != nil { return nil, crterrors.NewForbiddenError("invalid workspace request", err.Error()) } @@ -330,7 +330,7 @@ func (p *Proxy) processWorkspaceRequest(ctx echo.Context, userID, username, work // before proxying the request, verify that the user has a spacebinding for // the workspace and that the namespace -if any- belongs to the workspace - workspaces, err := p.listUserWorkspaces(ctx, workspaceName) + workspace, err := p.getUserWorkspaceWithBindings(ctx, workspaceName) if err != nil { return nil, err } @@ -338,12 +338,12 @@ func (p *Proxy) processWorkspaceRequest(ctx echo.Context, userID, username, work // check whether the user's has access to the home workspace // and whether the requestedNamespace -if any- exists in the workspace. requestedNamespace := namespaceFromCtx(ctx) - if err := validateWorkspaceRequest(workspaceName, requestedNamespace, workspaces); err != nil { + if err := validateWorkspaceRequest(workspaceName, requestedNamespace, *workspace); err != nil { return nil, crterrors.NewForbiddenError("invalid workspace request", err.Error()) } // retrieve the ClusterAccess for the user and the target workspace - return p.getClusterAccess(ctx, userID, username, workspaceName, proxyPluginName, workspaces) + return p.getClusterAccess(ctx, userID, username, workspaceName, proxyPluginName, workspace) } // validateSpaceAccess checks whether the user has access to the target workspace and that the Space exists. @@ -400,10 +400,9 @@ func (p *Proxy) validateUserAccess(ctx echo.Context, userID, username string) er // if the user has access to it. // Access can be either direct (an SpaceBinding linking the user to the workspace exists) // or community (a SpaceBinding linking the PublicViewer user to the workspace exists). -func (p *Proxy) getClusterAccess(ctx echo.Context, userID, username, workspaceName, proxyPluginName string, workspaces []toolchainv1alpha1.Workspace) (*access.ClusterAccess, error) { +func (p *Proxy) getClusterAccess(ctx echo.Context, userID, username, workspaceName, proxyPluginName string, workspace *toolchainv1alpha1.Workspace) (*access.ClusterAccess, error) { // retrieve by name the workspace from the list of workspaces the user has access to. - w := findWorkspaceByName(workspaceName, workspaces) - if w == nil { + if workspace == nil || workspace.Name != workspaceName { return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), "workspace not found") } @@ -415,7 +414,7 @@ func (p *Proxy) getClusterAccess(ctx echo.Context, userID, username, workspaceNa // if PublicViewer is enabled and user has no direct access to the workspace, proceed as PublicViewer publicViewerEnabled := context.IsPublicViewerEnabled(ctx) - if publicViewerEnabled && !hasDirectAccess(signup, w) { + if publicViewerEnabled && !hasDirectAccess(signup, workspace) { cluster, err := p.app.MemberClusterService().GetClusterAccess( toolchainv1alpha1.KubesawAuthenticatedUsername, toolchainv1alpha1.KubesawAuthenticatedUsername, @@ -451,18 +450,7 @@ func hasDirectAccess(signup *signup.Signup, workspace *toolchainv1alpha1.Workspa return false } -// findWorkspaceByName finds a workspace by name in a list of Workspaces. -func findWorkspaceByName(workspaceName string, workspaces []toolchainv1alpha1.Workspace) *toolchainv1alpha1.Workspace { - for _, w := range workspaces { - if w.Name == workspaceName { - w := w // TODO We won't need it after upgrading to go 1.22: https://go.dev/blog/loopvar-preview - return &w - } - } - return nil -} - -func (p *Proxy) listUserWorkspaces(ctx echo.Context, workspaceName string) ([]toolchainv1alpha1.Workspace, error) { +func (p *Proxy) getUserWorkspaceWithBindings(ctx echo.Context, workspaceName string) (*toolchainv1alpha1.Workspace, error) { // when a workspace name was provided // validate that the user has access to the workspace by getting all spacebindings recursively, starting from this workspace and going up to the parent workspaces till the "root" of the workspace tree. workspace, err := handlers.GetUserWorkspaceWithBindings(ctx, p.spaceLister, workspaceName, p.getMembersFunc) @@ -474,7 +462,7 @@ func (p *Proxy) listUserWorkspaces(ctx echo.Context, workspaceName string) ([]to return nil, crterrors.NewForbiddenError("invalid workspace request", fmt.Sprintf("access to workspace '%s' is forbidden", workspaceName)) } // workspace was found means we can forward the request - return []toolchainv1alpha1.Workspace{*workspace}, nil + return workspace, nil } func (p *Proxy) handleRequestAndRedirect(ctx echo.Context) error { @@ -812,7 +800,7 @@ func replaceTokenInWebsocketRequest(req *http.Request, newToken string) { // validateWorkspaceRequest checks whether the requested workspace is in the list of workspaces the user has visibility on (retrieved via the spaceLister). // If `requestedWorkspace` is zero, this function looks for the home workspace (the one with `status.Type` set to `home`). // If `requestedNamespace` is NOT zero, this function checks if the namespace exists in the workspace. -func validateWorkspaceRequest(requestedWorkspace, requestedNamespace string, workspaces []toolchainv1alpha1.Workspace) error { +func validateWorkspaceRequest(requestedWorkspace, requestedNamespace string, workspaces ...toolchainv1alpha1.Workspace) error { // check workspace access isHomeWSRequested := requestedWorkspace == "" diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index 0c5151df..572825f1 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -1115,7 +1115,7 @@ func (s *TestProxySuite) TestValidateWorkspaceRequest() { for k, tc := range tests { s.T().Run(k, func(t *testing.T) { - err := validateWorkspaceRequest(tc.requestedWorkspace, tc.requestedNamespace, tc.workspaces) + err := validateWorkspaceRequest(tc.requestedWorkspace, tc.requestedNamespace, tc.workspaces...) if tc.expectedErr == "" { require.NoError(t, err) } else { From ef8e333b659f5a8a17ee75b3fd78164bd35705c0 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 25 Jul 2024 10:56:03 +0200 Subject: [PATCH 25/70] refactor comments and function names Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 0737ce40..1cb2c0e9 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -323,19 +323,18 @@ func (p *Proxy) processHomeWorkspaceRequest(ctx echo.Context, userID, username, // processWorkspaceRequest process an HTTP Request targeting a specific workspace. func (p *Proxy) processWorkspaceRequest(ctx echo.Context, userID, username, workspaceName, proxyPluginName string) (*access.ClusterAccess, error) { - // check if the user has access to the target workspace - if err := p.validateSpaceAccess(ctx, userID, username, workspaceName); err != nil { + // check that the user is provisioned and the space exists + if err := p.validateUserIsProvisionedAndSpaceExists(ctx, userID, username, workspaceName); err != nil { return nil, err } - // before proxying the request, verify that the user has a spacebinding for - // the workspace and that the namespace -if any- belongs to the workspace + // retrieve the requested Workspace with SpaceBindings workspace, err := p.getUserWorkspaceWithBindings(ctx, workspaceName) if err != nil { return nil, err } - // check whether the user's has access to the home workspace + // check whether the user has access to the workspace // and whether the requestedNamespace -if any- exists in the workspace. requestedNamespace := namespaceFromCtx(ctx) if err := validateWorkspaceRequest(workspaceName, requestedNamespace, *workspace); err != nil { @@ -346,20 +345,20 @@ func (p *Proxy) processWorkspaceRequest(ctx echo.Context, userID, username, work return p.getClusterAccess(ctx, userID, username, workspaceName, proxyPluginName, workspace) } -// validateSpaceAccess checks whether the user has access to the target workspace and that the Space exists. -func (p *Proxy) validateSpaceAccess(ctx echo.Context, userID, username, workspaceName string) error { - if err := p.validateUserAccess(ctx, userID, username); err != nil { +// validateUserIsProvisionedAndSpaceExists checks that the user is provisioned and the Space exists. +// If the PublicViewer support is enabled, User check is skipped. +func (p *Proxy) validateUserIsProvisionedAndSpaceExists(ctx echo.Context, userID, username, workspaceName string) error { + if err := p.checkUserIsProvisioned(ctx, userID, username); err != nil { return crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) } - if err := p.validateSpace(workspaceName); err != nil { + if err := p.checkSpaceExists(workspaceName); err != nil { return crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) } - return nil } -// validateSpace checks whether the Space exists. -func (p *Proxy) validateSpace(workspaceName string) error { +// checkSpaceExists checks whether the Space exists. +func (p *Proxy) checkSpaceExists(workspaceName string) error { if _, err := p.app.InformerService().GetSpace(workspaceName); err != nil { // log the actual error but do not return it so that it doesn't reveal information about a space that may not belong to the requestor log.Error(nil, err, "unable to get target cluster for workspace "+workspaceName) @@ -368,9 +367,9 @@ func (p *Proxy) validateSpace(workspaceName string) error { return nil } -// validateUserAccess checks whether the user is Approved, if they are not an error is returned. +// checkUserIsProvisioned checks whether the user is Approved, if they are not an error is returned. // If public-viewer is enabled, user validation is skipped. -func (p *Proxy) validateUserAccess(ctx echo.Context, userID, username string) error { +func (p *Proxy) checkUserIsProvisioned(ctx echo.Context, userID, username string) error { // skip if public-viewer is enabled: read-only operations on community workspaces are always permitted. if context.IsPublicViewerEnabled(ctx) { return nil From 8cf0f7b30b21c23417567c0e94f6bc473b683212 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 25 Jul 2024 10:59:11 +0200 Subject: [PATCH 26/70] refactor Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 1cb2c0e9..14aca112 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -324,7 +324,7 @@ func (p *Proxy) processHomeWorkspaceRequest(ctx echo.Context, userID, username, // processWorkspaceRequest process an HTTP Request targeting a specific workspace. func (p *Proxy) processWorkspaceRequest(ctx echo.Context, userID, username, workspaceName, proxyPluginName string) (*access.ClusterAccess, error) { // check that the user is provisioned and the space exists - if err := p.validateUserIsProvisionedAndSpaceExists(ctx, userID, username, workspaceName); err != nil { + if err := p.checkUserIsProvisionedAndSpaceExists(ctx, userID, username, workspaceName); err != nil { return nil, err } @@ -345,9 +345,9 @@ func (p *Proxy) processWorkspaceRequest(ctx echo.Context, userID, username, work return p.getClusterAccess(ctx, userID, username, workspaceName, proxyPluginName, workspace) } -// validateUserIsProvisionedAndSpaceExists checks that the user is provisioned and the Space exists. +// checkUserIsProvisionedAndSpaceExists checks that the user is provisioned and the Space exists. // If the PublicViewer support is enabled, User check is skipped. -func (p *Proxy) validateUserIsProvisionedAndSpaceExists(ctx echo.Context, userID, username, workspaceName string) error { +func (p *Proxy) checkUserIsProvisionedAndSpaceExists(ctx echo.Context, userID, username, workspaceName string) error { if err := p.checkUserIsProvisioned(ctx, userID, username); err != nil { return crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) } From 29dd7fe98a7249d07874910c40f6076b6c6afc03 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 25 Jul 2024 12:48:14 +0200 Subject: [PATCH 27/70] fix comments Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 14aca112..b5c10284 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -397,7 +397,7 @@ func (p *Proxy) checkUserIsProvisioned(ctx echo.Context, userID, username string // getClusterAccess retrieves the access to the cluster hosting the requested workspace, // if the user has access to it. -// Access can be either direct (an SpaceBinding linking the user to the workspace exists) +// Access can be either direct (a SpaceBinding linking the user to the workspace exists) // or community (a SpaceBinding linking the PublicViewer user to the workspace exists). func (p *Proxy) getClusterAccess(ctx echo.Context, userID, username, workspaceName, proxyPluginName string, workspace *toolchainv1alpha1.Workspace) (*access.ClusterAccess, error) { // retrieve by name the workspace from the list of workspaces the user has access to. @@ -451,7 +451,8 @@ func hasDirectAccess(signup *signup.Signup, workspace *toolchainv1alpha1.Workspa func (p *Proxy) getUserWorkspaceWithBindings(ctx echo.Context, workspaceName string) (*toolchainv1alpha1.Workspace, error) { // when a workspace name was provided - // validate that the user has access to the workspace by getting all spacebindings recursively, starting from this workspace and going up to the parent workspaces till the "root" of the workspace tree. + // validate that the user has access to the workspace by getting all spacebindings recursively, + // starting from this workspace and going up to the parent workspaces till the "root" of the workspace tree. workspace, err := handlers.GetUserWorkspaceWithBindings(ctx, p.spaceLister, workspaceName, p.getMembersFunc) if err != nil { return nil, crterrors.NewInternalError(errs.New("unable to retrieve user workspaces"), err.Error()) From 00dd2f4a32e3c3719c53b4b1f3e4f6962e19e703 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 25 Jul 2024 12:48:21 +0200 Subject: [PATCH 28/70] improve code readability Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index b5c10284..ab4ac316 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -405,33 +405,38 @@ func (p *Proxy) getClusterAccess(ctx echo.Context, userID, username, workspaceNa return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), "workspace not found") } + // retrieve cluster access as requesting user or PublicViewer + cluster, err := p.getClusterAccessAsUserOrPublicViewer(ctx, userID, username, proxyPluginName, workspace) + if err != nil { + return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) + } + return cluster, nil +} + +// getClusterAccessAsUserOrPublicViewer if the requesting user has direct access to the workspace, +// it returns the ClusterAccess impersonating the requesting user. +// Otherwise, if PublicViewer support is enabled and PublicViewer user has access to the workspace, +// it returns the ClusterAccess impersonating the PublicViewer user. +func (p *Proxy) getClusterAccessAsUserOrPublicViewer(ctx echo.Context, userID, username, proxyPluginName string, workspace *toolchainv1alpha1.Workspace) (*access.ClusterAccess, error) { // retrieve the requesting user's UserSignup - signup, err := p.spaceLister.GetSignupFunc(nil, userID, username, false) + userSignup, err := p.spaceLister.GetSignupFunc(nil, userID, username, false) if err != nil { return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), "user not found") } - // if PublicViewer is enabled and user has no direct access to the workspace, proceed as PublicViewer + // proceed as PublicViewer if needed publicViewerEnabled := context.IsPublicViewerEnabled(ctx) - if publicViewerEnabled && !hasDirectAccess(signup, workspace) { - cluster, err := p.app.MemberClusterService().GetClusterAccess( + if publicViewerEnabled && !hasDirectAccess(userSignup, workspace) { + return p.app.MemberClusterService().GetClusterAccess( toolchainv1alpha1.KubesawAuthenticatedUsername, toolchainv1alpha1.KubesawAuthenticatedUsername, - workspaceName, + workspace.Name, proxyPluginName, publicViewerEnabled) - if err != nil { - return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) - } - return cluster, nil } // retrieve the ClusterAccess for the cluster hosting the workspace and the given user. - cluster, err := p.app.MemberClusterService().GetClusterAccess(userID, username, workspaceName, proxyPluginName, publicViewerEnabled) - if err != nil { - return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) - } - return cluster, nil + return p.app.MemberClusterService().GetClusterAccess(userID, username, workspace.Name, proxyPluginName, publicViewerEnabled) } // hasDirectAccess checks if an UserSignup has access to a workspace. From f9b9b5a0fd1166c3f38d05f4ce80006866522624 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 25 Jul 2024 13:00:29 +0200 Subject: [PATCH 29/70] improve cluster_service code reuse Signed-off-by: Francesco Ilario --- pkg/proxy/service/cluster_service.go | 32 ++++++++++++++++------------ 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/pkg/proxy/service/cluster_service.go b/pkg/proxy/service/cluster_service.go index ca7ca26f..586afb0a 100644 --- a/pkg/proxy/service/cluster_service.go +++ b/pkg/proxy/service/cluster_service.go @@ -11,6 +11,7 @@ import ( servicecontext "github.com/codeready-toolchain/registration-service/pkg/application/service/context" "github.com/codeready-toolchain/registration-service/pkg/log" "github.com/codeready-toolchain/registration-service/pkg/proxy/access" + "github.com/codeready-toolchain/registration-service/pkg/signup" "github.com/codeready-toolchain/toolchain-common/pkg/cluster" routev1 "github.com/openshift/api/route/v1" @@ -75,39 +76,42 @@ func (s *ServiceImpl) getUserSignupComplaintName(userID, username string, public } // retrieve the UserSignup from cache - // - // don't check for usersignup complete status, since it might cause the proxy blocking the request - // and returning an error when quick transitions from ready to provisioning are happening. - userSignup, err := s.Services().SignupService().GetSignupFromInformer(nil, userID, username, false) + userSignup, err := s.getSignupFromInformerForProvisionedUser(userID, username) if err != nil { return "", err } - // if signup has the CompliantUsername set it means that MUR was created and useraccount is provisioned - if userSignup == nil || userSignup.CompliantUsername == "" { - cause := errs.New("user is not provisioned (yet)") - log.Error(nil, cause, fmt.Sprintf("signup object: %+v", userSignup)) - return "", cause - } return userSignup.CompliantUsername, nil } // getClusterAccessForDefaultWorkspace retrieves the cluster for the user's default workspace func (s *ServiceImpl) getClusterAccessForDefaultWorkspace(userID, username, proxyPluginName string) (*access.ClusterAccess, error) { + // retrieve the UserSignup from cache + userSignup, err := s.getSignupFromInformerForProvisionedUser(userID, username) + if err != nil { + return nil, err + } + + // retrieve user's access for cluster + return s.accessForCluster(userSignup.APIEndpoint, userSignup.ClusterName, userSignup.CompliantUsername, proxyPluginName) +} + +func (s *ServiceImpl) getSignupFromInformerForProvisionedUser(userID, username string) (*signup.Signup, error) { // don't check for usersignup complete status, since it might cause the proxy blocking the request // and returning an error when quick transitions from ready to provisioning are happening. - signup, err := s.Services().SignupService().GetSignupFromInformer(nil, userID, username, false) + userSignup, err := s.Services().SignupService().GetSignupFromInformer(nil, userID, username, false) if err != nil { return nil, err } + // if signup has the CompliantUsername set it means that MUR was created and useraccount is provisioned - if signup == nil || signup.CompliantUsername == "" { + if userSignup == nil || userSignup.CompliantUsername == "" { cause := errs.New("user is not provisioned (yet)") - log.Error(nil, cause, fmt.Sprintf("signup object: %+v", signup)) + log.Error(nil, cause, fmt.Sprintf("signup object: %+v", userSignup)) return nil, cause } - return s.accessForCluster(signup.APIEndpoint, signup.ClusterName, signup.CompliantUsername, proxyPluginName) + return userSignup, nil } func (s *ServiceImpl) accessForSpace(space *toolchainv1alpha1.Space, username, proxyPluginName string) (*access.ClusterAccess, error) { From c683c659a88baf7c5f18a403a064c3b760619864 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 25 Jul 2024 14:33:24 +0200 Subject: [PATCH 30/70] Add pkg/context tests for public-viewer Signed-off-by: Francesco Ilario --- pkg/context/public_viewer.go | 4 ++- pkg/context/public_viewer_test.go | 53 +++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 pkg/context/public_viewer_test.go diff --git a/pkg/context/public_viewer.go b/pkg/context/public_viewer.go index f0b14e03..d7f11b23 100644 --- a/pkg/context/public_viewer.go +++ b/pkg/context/public_viewer.go @@ -1,6 +1,8 @@ package context -import "github.com/labstack/echo/v4" +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. 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 3452b2be8c39b121783bc8756b19e0a5e36f1b0c Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 12 Aug 2024 17:12:44 +0200 Subject: [PATCH 31/70] improve tests Signed-off-by: Francesco Ilario --- pkg/proxy/handlers/spacelister_get_test.go | 28 +++++++++++++++++-- pkg/proxy/proxy_community_test.go | 32 ++++++++++++++++++++-- 2 files changed, 55 insertions(+), 5 deletions(-) diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index 086809a5..35aa5ef5 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -748,6 +748,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, @@ -797,11 +798,16 @@ func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { expectedWorkspace: nil, expectedErr: "", }, - "gordon can get batman workspace": { - username: "gordon.space", + "gordon can get robin workspace": { + 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 { @@ -848,11 +854,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, @@ -927,6 +934,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 { diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index 8e2ed5e6..0a47165e 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -68,6 +68,8 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr s.Run("successfully proxy", func() { owner := uuid.New() communityUser := uuid.New() + alice := uuid.New() + notReadyUser := uuid.New() httpTestServerResponse := "my response" // Start the member-2 API Server @@ -110,6 +112,24 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr RequestPath: fmt.Sprintf("http://localhost:%s/workspaces/communityspace/api/communityspace/pods", port), ExpectedResponse: httpTestServerResponse, }, + // Given A not ready user exists + // When the not ready user requests the list of pods in workspace communityspace + // Then the request is forwarded from the proxy + // And the request impersonates the not ready user + // And the request's X-SSO-User Header is set to not ready user's ID + // And the request is successful + "plain http actual request as notReadyUser": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(notReadyUser)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {toolchainv1alpha1.KubesawAuthenticatedUsername}, + "X-SSO-User": {"username-" + notReadyUser.String()}, + }, + ExpectedProxyResponseStatus: http.StatusOK, + RequestPath: fmt.Sprintf("http://localhost:%s/workspaces/communityspace/api/communityspace/pods", port), + ExpectedResponse: httpTestServerResponse, + }, // Given smith2 owns a workspace named communityspace // And communityspace is publicly visible (shared with PublicViewer) // And a user named communityuser exists @@ -187,13 +207,13 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr Name: "communityUser", APIEndpoint: testServer.URL, ClusterName: "member-2", - CompliantUsername: "communityuser", + CompliantUsername: "communityUser", Username: "communityUser@", Status: signup.Status{ Ready: true, }, }), - fake.Signup(communityUser.String(), &signup.Signup{ + fake.Signup(alice.String(), &signup.Signup{ Name: "alice", APIEndpoint: testServer.URL, ClusterName: "member-2", @@ -203,6 +223,14 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr Ready: true, }, }), + fake.Signup(notReadyUser.String(), &signup.Signup{ + Name: "notReadyUser", + CompliantUsername: "notReadyUser", + Username: "notReadyUser@", + Status: signup.Status{ + Ready: false, + }, + }), ) s.Application.MockSignupService(fakeApp.SignupServiceMock) inf := fake.NewFakeInformer() From 2b62f6c8037a8aee8d16ee6ee3116756672e1d95 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 26 Aug 2024 14:57:50 +0200 Subject: [PATCH 32/70] rollback changes to pkg/server/in_cluster_application.go Signed-off-by: Francesco Ilario --- pkg/server/in_cluster_application.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/server/in_cluster_application.go b/pkg/server/in_cluster_application.go index ad11922b..979cba2d 100644 --- a/pkg/server/in_cluster_application.go +++ b/pkg/server/in_cluster_application.go @@ -29,8 +29,7 @@ func NewInClusterApplication(informer informers.Informer) (application.Applicati serviceFactory: factory.NewServiceFactory( factory.WithServiceContextOptions(factory.CRTClientOption(kubeClient), factory.InformerOption(informer), - ), - ), + )), }, nil } From 3ccbee705c5696a67ed5aea374226c5cee460f4f Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 1 Aug 2024 11:29:26 +0200 Subject: [PATCH 33/70] add PublicViewer's middleware this commit is part of the effort of splitting #443 Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index ab4ac316..89791ed7 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -119,6 +119,7 @@ func (p *Proxy) StartProxy(port string) *http.Server { return next(ctx) } }, + p.addPublicViewerContext(), ) // middleware after routing @@ -568,6 +569,17 @@ func (p *Proxy) addUserContext() echo.MiddlewareFunc { } } +// addPublicViewerContext updates echo.Context with the configuration's PublicViewerEnabled value. +func (p *Proxy) addPublicViewerContext() echo.MiddlewareFunc { + return func(next echo.HandlerFunc) echo.HandlerFunc { + return func(ctx echo.Context) error { + publicViewerEnabled := configuration.GetRegistrationServiceConfig().PublicViewerEnabled() + ctx.Set(context.PublicViewerEnabled, publicViewerEnabled) + return next(ctx) + } + } +} + func (p *Proxy) stripInvalidHeaders() echo.MiddlewareFunc { return func(next echo.HandlerFunc) echo.HandlerFunc { return func(ctx echo.Context) error { From 276323b77721a2b9ec447632d571e700825c1003 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 26 Aug 2024 19:31:29 +0200 Subject: [PATCH 34/70] remove ctx set in favor of middleware Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 89791ed7..21b0e97f 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -473,8 +473,6 @@ func (p *Proxy) getUserWorkspaceWithBindings(ctx echo.Context, workspaceName str func (p *Proxy) handleRequestAndRedirect(ctx echo.Context) error { requestReceivedTime := ctx.Get(context.RequestReceivedTime).(time.Time) - publicViewerEnabled := configuration.GetRegistrationServiceConfig().PublicViewerEnabled() - ctx.Set(context.PublicViewerEnabled, publicViewerEnabled) proxyPluginName, cluster, err := p.processRequest(ctx) if err != nil { p.metrics.RegServProxyAPIHistogramVec.WithLabelValues(fmt.Sprintf("%d", http.StatusNotAcceptable), metrics.MetricLabelRejected).Observe(time.Since(requestReceivedTime).Seconds()) From 8d944fb19e69a25eb1e4a168fc56ae3cdcf10969 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 26 Aug 2024 19:45:32 +0200 Subject: [PATCH 35/70] fix error message Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 21b0e97f..6de9eb4e 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -362,7 +362,7 @@ func (p *Proxy) checkUserIsProvisionedAndSpaceExists(ctx echo.Context, userID, u func (p *Proxy) checkSpaceExists(workspaceName string) error { if _, err := p.app.InformerService().GetSpace(workspaceName); err != nil { // log the actual error but do not return it so that it doesn't reveal information about a space that may not belong to the requestor - log.Error(nil, err, "unable to get target cluster for workspace "+workspaceName) + log.Errorf(nil, err, "requested space '%s' is not available", workspaceName) return fmt.Errorf("the requested space is not available") } return nil From f74ee96c65304e66ab6fca1d7421fc67c26cfcb0 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 27 Aug 2024 11:00:00 +0200 Subject: [PATCH 36/70] Update pkg/proxy/proxy.go Co-authored-by: Alexey Kazakov --- pkg/proxy/proxy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 6de9eb4e..1808eda5 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -311,7 +311,7 @@ func (p *Proxy) processHomeWorkspaceRequest(ctx echo.Context, userID, username, return nil, crterrors.NewInternalError(errs.New("unable to retrieve user workspaces"), err.Error()) } - // check whether the user's has access to the home workspace + // check whether the user has access to the home workspace // and whether the requestedNamespace -if any- exists in the workspace. requestedNamespace := namespaceFromCtx(ctx) if err := validateWorkspaceRequest("", requestedNamespace, workspaces...); err != nil { From d278ea789d4b2ebcb7f4cd563b613b34d7c85ab6 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 27 Aug 2024 11:25:27 +0200 Subject: [PATCH 37/70] remove not needed checks Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 1808eda5..5a59978d 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -402,7 +402,7 @@ func (p *Proxy) checkUserIsProvisioned(ctx echo.Context, userID, username string // or community (a SpaceBinding linking the PublicViewer user to the workspace exists). func (p *Proxy) getClusterAccess(ctx echo.Context, userID, username, workspaceName, proxyPluginName string, workspace *toolchainv1alpha1.Workspace) (*access.ClusterAccess, error) { // retrieve by name the workspace from the list of workspaces the user has access to. - if workspace == nil || workspace.Name != workspaceName { + if workspace == nil { return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), "workspace not found") } @@ -443,7 +443,7 @@ func (p *Proxy) getClusterAccessAsUserOrPublicViewer(ctx echo.Context, userID, u // hasDirectAccess checks if an UserSignup has access to a workspace. // Workspace's bindings are obtained from its `status.bindings` property. func hasDirectAccess(signup *signup.Signup, workspace *toolchainv1alpha1.Workspace) bool { - if signup == nil || workspace == nil { + if signup == nil { return false } From 6f97b34a1bbc426c89a01af42e7c5ca71ba3b7fc Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 27 Aug 2024 15:06:23 +0200 Subject: [PATCH 38/70] remove unused parameter Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 5a59978d..ee916913 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -343,7 +343,7 @@ func (p *Proxy) processWorkspaceRequest(ctx echo.Context, userID, username, work } // retrieve the ClusterAccess for the user and the target workspace - return p.getClusterAccess(ctx, userID, username, workspaceName, proxyPluginName, workspace) + return p.getClusterAccess(ctx, userID, username, proxyPluginName, workspace) } // checkUserIsProvisionedAndSpaceExists checks that the user is provisioned and the Space exists. @@ -400,7 +400,7 @@ func (p *Proxy) checkUserIsProvisioned(ctx echo.Context, userID, username string // if the user has access to it. // Access can be either direct (a SpaceBinding linking the user to the workspace exists) // or community (a SpaceBinding linking the PublicViewer user to the workspace exists). -func (p *Proxy) getClusterAccess(ctx echo.Context, userID, username, workspaceName, proxyPluginName string, workspace *toolchainv1alpha1.Workspace) (*access.ClusterAccess, error) { +func (p *Proxy) getClusterAccess(ctx echo.Context, userID, username, proxyPluginName string, workspace *toolchainv1alpha1.Workspace) (*access.ClusterAccess, error) { // retrieve by name the workspace from the list of workspaces the user has access to. if workspace == nil { return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), "workspace not found") From 7934119676d27a9cb76a3587419303cc24c612e7 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Wed, 28 Aug 2024 19:54:15 +0200 Subject: [PATCH 39/70] Update pkg/proxy/proxy.go Co-authored-by: Alexey Kazakov --- pkg/proxy/proxy.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index ee916913..ae729150 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -362,8 +362,8 @@ func (p *Proxy) checkUserIsProvisionedAndSpaceExists(ctx echo.Context, userID, u func (p *Proxy) checkSpaceExists(workspaceName string) error { if _, err := p.app.InformerService().GetSpace(workspaceName); err != nil { // log the actual error but do not return it so that it doesn't reveal information about a space that may not belong to the requestor - log.Errorf(nil, err, "requested space '%s' is not available", workspaceName) - return fmt.Errorf("the requested space is not available") + log.Errorf(nil, err, "requested space '%s' does not exist", workspaceName) + return fmt.Errorf("access to workspace '%s' is forbidden", workspaceName) } return nil } From 30b980bd03aefad976de496610a62258fc3d75c5 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 3 Sep 2024 11:23:47 +0200 Subject: [PATCH 40/70] fix tests Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index 572825f1..b01bb37a 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -606,7 +606,7 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy, publ ExpectedAPIServerRequestHeaders: map[string][]string{ "Authorization": {"Bearer clusterSAToken"}, }, - ExpectedResponse: ptr("unable to get target cluster: the requested space is not available"), + ExpectedResponse: ptr("unable to get target cluster: access to workspace 'not-existing-workspace' is forbidden"), ExpectedProxyResponseStatus: http.StatusInternalServerError, }, } From 51fe1a41c26a38060c0c6302e1bab78866e3e97a Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 3 Sep 2024 15:08:24 +0200 Subject: [PATCH 41/70] improve comments Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index ae729150..ad8d09d4 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -324,7 +324,8 @@ func (p *Proxy) processHomeWorkspaceRequest(ctx echo.Context, userID, username, // processWorkspaceRequest process an HTTP Request targeting a specific workspace. func (p *Proxy) processWorkspaceRequest(ctx echo.Context, userID, username, workspaceName, proxyPluginName string) (*access.ClusterAccess, error) { - // check that the user is provisioned and the space exists + // check that the user is provisioned and the space exists. + // if the PublicViewer support is enabled, user check is skipped. if err := p.checkUserIsProvisionedAndSpaceExists(ctx, userID, username, workspaceName); err != nil { return nil, err } From e27d558f79c0b1af02684565c8a28b4b0663b879 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 3 Sep 2024 15:08:43 +0200 Subject: [PATCH 42/70] remove redundant check Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index ad8d09d4..b04a3ce4 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -402,11 +402,6 @@ func (p *Proxy) checkUserIsProvisioned(ctx echo.Context, userID, username string // Access can be either direct (a SpaceBinding linking the user to the workspace exists) // or community (a SpaceBinding linking the PublicViewer user to the workspace exists). func (p *Proxy) getClusterAccess(ctx echo.Context, userID, username, proxyPluginName string, workspace *toolchainv1alpha1.Workspace) (*access.ClusterAccess, error) { - // retrieve by name the workspace from the list of workspaces the user has access to. - if workspace == nil { - return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), "workspace not found") - } - // retrieve cluster access as requesting user or PublicViewer cluster, err := p.getClusterAccessAsUserOrPublicViewer(ctx, userID, username, proxyPluginName, workspace) if err != nil { From 96b16461e85e6a6d7c89800812167c2bf6ab6f29 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 3 Sep 2024 15:09:08 +0200 Subject: [PATCH 43/70] improve returned error and logging Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index b04a3ce4..ab2c2386 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -418,7 +418,8 @@ func (p *Proxy) getClusterAccessAsUserOrPublicViewer(ctx echo.Context, userID, u // retrieve the requesting user's UserSignup userSignup, err := p.spaceLister.GetSignupFunc(nil, userID, username, false) if err != nil { - return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), "user not found") + log.Error(nil, err, fmt.Sprintf("error retrieving user signup for userID '%s' and username '%s'", userID, username)) + return nil, crterrors.NewInternalError(errs.New("unable to get user info"), "error retrieving user") } // proceed as PublicViewer if needed From 048f29ba1bc98e564f62713f6bc277a8d2e90e09 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 3 Sep 2024 15:14:45 +0200 Subject: [PATCH 44/70] improve comments Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index ab2c2386..9cab0d2b 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -410,10 +410,12 @@ func (p *Proxy) getClusterAccess(ctx echo.Context, userID, username, proxyPlugin return cluster, nil } -// getClusterAccessAsUserOrPublicViewer if the requesting user has direct access to the workspace, -// it returns the ClusterAccess impersonating the requesting user. -// Otherwise, if PublicViewer support is enabled and PublicViewer user has access to the workspace, -// it returns the ClusterAccess impersonating the PublicViewer user. +// getClusterAccessAsUserOrPublicViewer if the requesting user exists and has direct access to the workspace, +// this function returns the ClusterAccess impersonating the requesting user. +// If PublicViewer support is enabled and PublicViewer user has access to the workspace, +// this function returns the ClusterAccess impersonating the PublicViewer user. +// If requesting user does not exists and PublicViewer is disabled or does not have access to the workspace, +// this function returns an error. func (p *Proxy) getClusterAccessAsUserOrPublicViewer(ctx echo.Context, userID, username, proxyPluginName string, workspace *toolchainv1alpha1.Workspace) (*access.ClusterAccess, error) { // retrieve the requesting user's UserSignup userSignup, err := p.spaceLister.GetSignupFunc(nil, userID, username, false) @@ -422,7 +424,7 @@ func (p *Proxy) getClusterAccessAsUserOrPublicViewer(ctx echo.Context, userID, u return nil, crterrors.NewInternalError(errs.New("unable to get user info"), "error retrieving user") } - // proceed as PublicViewer if needed + // proceed as PublicViewer if the feature is enabled and userSignup is nil publicViewerEnabled := context.IsPublicViewerEnabled(ctx) if publicViewerEnabled && !hasDirectAccess(userSignup, workspace) { return p.app.MemberClusterService().GetClusterAccess( @@ -433,7 +435,7 @@ func (p *Proxy) getClusterAccessAsUserOrPublicViewer(ctx echo.Context, userID, u publicViewerEnabled) } - // retrieve the ClusterAccess for the cluster hosting the workspace and the given user. + // otherwise retrieve the ClusterAccess for the cluster hosting the workspace and the given user. return p.app.MemberClusterService().GetClusterAccess(userID, username, workspace.Name, proxyPluginName, publicViewerEnabled) } From d9299c5d535c79b46e772678397fd0473a7db0c8 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 3 Sep 2024 15:18:58 +0200 Subject: [PATCH 45/70] improve comments Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 9cab0d2b..75ec3fe7 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -454,10 +454,10 @@ func hasDirectAccess(signup *signup.Signup, workspace *toolchainv1alpha1.Workspa return false } +// getUserWorkspaceWithBindings retrieves the workspace with the SpaceBindings if the requesting user has access to it. +// User access to the Workspace is checked by getting all spacebindings recursively, +// starting from this workspace and going up to the parent workspaces till the "root" of the workspace tree. func (p *Proxy) getUserWorkspaceWithBindings(ctx echo.Context, workspaceName string) (*toolchainv1alpha1.Workspace, error) { - // when a workspace name was provided - // validate that the user has access to the workspace by getting all spacebindings recursively, - // starting from this workspace and going up to the parent workspaces till the "root" of the workspace tree. workspace, err := handlers.GetUserWorkspaceWithBindings(ctx, p.spaceLister, workspaceName, p.getMembersFunc) if err != nil { return nil, crterrors.NewInternalError(errs.New("unable to retrieve user workspaces"), err.Error()) From 6408242518acad1da5c980985948e8af5dd8b25f Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Tue, 3 Sep 2024 16:31:37 +0200 Subject: [PATCH 46/70] check if public-viewer has access to space before forwarding Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 75ec3fe7..816a4baa 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -426,7 +426,11 @@ func (p *Proxy) getClusterAccessAsUserOrPublicViewer(ctx echo.Context, userID, u // proceed as PublicViewer if the feature is enabled and userSignup is nil publicViewerEnabled := context.IsPublicViewerEnabled(ctx) - if publicViewerEnabled && !hasDirectAccess(userSignup, workspace) { + if publicViewerEnabled && !userHasDirectAccess(userSignup, workspace) { + if !publicViewerHasAccess(workspace) { + return nil, crterrors.NewForbiddenError("invalid workspace request", fmt.Sprintf("access to workspace '%s' is forbidden", workspace.Name)) + } + return p.app.MemberClusterService().GetClusterAccess( toolchainv1alpha1.KubesawAuthenticatedUsername, toolchainv1alpha1.KubesawAuthenticatedUsername, @@ -439,19 +443,28 @@ func (p *Proxy) getClusterAccessAsUserOrPublicViewer(ctx echo.Context, userID, u return p.app.MemberClusterService().GetClusterAccess(userID, username, workspace.Name, proxyPluginName, publicViewerEnabled) } -// hasDirectAccess checks if an UserSignup has access to a workspace. +func publicViewerHasAccess(workspace *toolchainv1alpha1.Workspace) bool { + return userHasBinding(toolchainv1alpha1.KubesawAuthenticatedUsername, workspace) +} + +// userHasDirectAccess checks if an UserSignup has access to a workspace. // Workspace's bindings are obtained from its `status.bindings` property. -func hasDirectAccess(signup *signup.Signup, workspace *toolchainv1alpha1.Workspace) bool { +func userHasDirectAccess(signup *signup.Signup, workspace *toolchainv1alpha1.Workspace) bool { if signup == nil { return false } + return userHasBinding(signup.CompliantUsername, workspace) +} + +func userHasBinding(username string, workspace *toolchainv1alpha1.Workspace) bool { for _, b := range workspace.Status.Bindings { - if b.MasterUserRecord == signup.CompliantUsername { + if b.MasterUserRecord == username { return true } } return false + } // getUserWorkspaceWithBindings retrieves the workspace with the SpaceBindings if the requesting user has access to it. From a2bc3f8d7d236840510c35e201c98caf1bc0873c Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 12 Sep 2024 09:54:43 +0200 Subject: [PATCH 47/70] add test for not signed up user Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_community_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index 0a47165e..538cd55e 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -70,6 +70,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr communityUser := uuid.New() alice := uuid.New() notReadyUser := uuid.New() + notSignedUpUser := uuid.New() httpTestServerResponse := "my response" // Start the member-2 API Server @@ -130,6 +131,24 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr RequestPath: fmt.Sprintf("http://localhost:%s/workspaces/communityspace/api/communityspace/pods", port), ExpectedResponse: httpTestServerResponse, }, + // Given A not signed up user exists + // When the not signed up user requests the list of pods in workspace communityspace + // Then the request is forwarded from the proxy + // And the request impersonates the not ready user + // And the request's X-SSO-User Header is set to not signed up user's ID + // And the request is successful + "plain http actual request as notSignedUpUser": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(notSignedUpUser)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {toolchainv1alpha1.KubesawAuthenticatedUsername}, + "X-SSO-User": {"username-" + notSignedUpUser.String()}, + }, + ExpectedProxyResponseStatus: http.StatusOK, + RequestPath: fmt.Sprintf("http://localhost:%s/workspaces/communityspace/api/communityspace/pods", port), + ExpectedResponse: httpTestServerResponse, + }, // Given smith2 owns a workspace named communityspace // And communityspace is publicly visible (shared with PublicViewer) // And a user named communityuser exists From 47ab59e92f21ed93617108b663162bc6035f790e Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 12 Sep 2024 09:57:01 +0200 Subject: [PATCH 48/70] fix typo in comment Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_community_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index 538cd55e..f786befd 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -134,7 +134,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr // Given A not signed up user exists // When the not signed up user requests the list of pods in workspace communityspace // Then the request is forwarded from the proxy - // And the request impersonates the not ready user + // And the request impersonates the not signed up user // And the request's X-SSO-User Header is set to not signed up user's ID // And the request is successful "plain http actual request as notSignedUpUser": { From d4fee9b9308043bec087b53dd1328c68771f99db Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 12 Sep 2024 10:20:53 +0200 Subject: [PATCH 49/70] refactor user names and cleanup test Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_community_test.go | 95 ++++++++++++++----------------- 1 file changed, 44 insertions(+), 51 deletions(-) diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index f786befd..0d3081e8 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -66,8 +66,7 @@ func (s *TestProxySuite) TestProxyCommunityEnabled() { func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Proxy, port string) { s.Run("successfully proxy", func() { - owner := uuid.New() - communityUser := uuid.New() + smith := uuid.New() alice := uuid.New() notReadyUser := uuid.New() notSignedUpUser := uuid.New() @@ -93,28 +92,32 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr ExpectedResponse string } + podsRequestUrl := func(workspace string) string { + return fmt.Sprintf("http://localhost:%s/workspaces/%s/api/pods", port, workspace) + } + tests := map[string]testCase{ - // Given smith2 owns a workspace named communityspace - // And communityspace is publicly visible (shared with PublicViewer) - // When smith2 requests the list of pods in workspace communityspace + // Given smith owns a workspace named smith-community + // And smith-community is publicly visible (shared with PublicViewer) + // When smith requests the list of pods in workspace smith-community // Then the request is forwarded from the proxy - // And the request impersonates smith2 - // And the request's X-SSO-User Header is set to smith2's ID + // And the request impersonates smith + // And the request's X-SSO-User Header is set to smith's ID // And the request is successful - "plain http actual request as owner": { + "plain http actual request as community space owner": { ProxyRequestMethod: "GET", - ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(owner)}}, + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(smith)}}, ExpectedAPIServerRequestHeaders: map[string][]string{ "Authorization": {"Bearer clusterSAToken"}, - "Impersonate-User": {"smith2"}, - "X-SSO-User": {"username-" + owner.String()}, + "Impersonate-User": {"smith"}, + "X-SSO-User": {"username-" + smith.String()}, }, ExpectedProxyResponseStatus: http.StatusOK, - RequestPath: fmt.Sprintf("http://localhost:%s/workspaces/communityspace/api/communityspace/pods", port), + RequestPath: podsRequestUrl("smith-community"), ExpectedResponse: httpTestServerResponse, }, // Given A not ready user exists - // When the not ready user requests the list of pods in workspace communityspace + // When the not ready user requests the list of pods in workspace smith-community // Then the request is forwarded from the proxy // And the request impersonates the not ready user // And the request's X-SSO-User Header is set to not ready user's ID @@ -128,11 +131,11 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr "X-SSO-User": {"username-" + notReadyUser.String()}, }, ExpectedProxyResponseStatus: http.StatusOK, - RequestPath: fmt.Sprintf("http://localhost:%s/workspaces/communityspace/api/communityspace/pods", port), + RequestPath: podsRequestUrl("smith-community"), ExpectedResponse: httpTestServerResponse, }, // Given A not signed up user exists - // When the not signed up user requests the list of pods in workspace communityspace + // When the not signed up user requests the list of pods in workspace smith-community // Then the request is forwarded from the proxy // And the request impersonates the not signed up user // And the request's X-SSO-User Header is set to not signed up user's ID @@ -146,47 +149,47 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr "X-SSO-User": {"username-" + notSignedUpUser.String()}, }, ExpectedProxyResponseStatus: http.StatusOK, - RequestPath: fmt.Sprintf("http://localhost:%s/workspaces/communityspace/api/communityspace/pods", port), + RequestPath: podsRequestUrl("smith-community"), ExpectedResponse: httpTestServerResponse, }, - // Given smith2 owns a workspace named communityspace - // And communityspace is publicly visible (shared with PublicViewer) + // Given smith owns a workspace named smith-community + // And smith-community is publicly visible (shared with PublicViewer) // And a user named communityuser exists - // And smith2's communityspace is not directly shared with communityuser - // When communityuser requests the list of pods in workspace communityspace + // And smith's smith-community is not directly shared with communityuser + // When communityuser requests the list of pods in workspace smith-community // Then the request is forwarded from the proxy // And the request impersonates the PublicViewer // And the request's X-SSO-User Header is set to communityuser's ID // And the request is successful "plain http actual request as community user": { ProxyRequestMethod: "GET", - ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(communityUser)}}, + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(alice)}}, ExpectedAPIServerRequestHeaders: map[string][]string{ "Authorization": {"Bearer clusterSAToken"}, "Impersonate-User": {toolchainv1alpha1.KubesawAuthenticatedUsername}, - "X-SSO-User": {"username-" + communityUser.String()}, + "X-SSO-User": {"username-" + alice.String()}, }, ExpectedProxyResponseStatus: http.StatusOK, - RequestPath: fmt.Sprintf("http://localhost:%s/workspaces/communityspace/api/communityspace/pods", port), + RequestPath: podsRequestUrl("smith-community"), ExpectedResponse: httpTestServerResponse, }, // Given user alice exists // And alice owns a private workspace - // When smith2 requests the list of pods in alice's workspace + // When smith requests the list of pods in alice's workspace // Then the request is forwarded from the proxy - // And the request impersonates smith2 - // And the request's X-SSO-User Header is set to smith2's ID + // And the request impersonates smith + // And the request's X-SSO-User Header is set to smith's ID // And the request is NOT successful - "plain http actual request as not owner to private workspace": { + "plain http actual request as non-owner to private workspace": { ProxyRequestMethod: "GET", - ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(owner)}}, + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(smith)}}, ExpectedAPIServerRequestHeaders: map[string][]string{ "Authorization": {"Bearer clusterSAToken"}, - "Impersonate-User": {"smith2"}, - "X-SSO-User": {"username-" + owner.String()}, + "Impersonate-User": {"smith"}, + "X-SSO-User": {"username-" + smith.String()}, }, ExpectedProxyResponseStatus: http.StatusForbidden, - RequestPath: fmt.Sprintf("http://localhost:%s/workspaces/alice-private/api/alice-private/pods", port), + RequestPath: podsRequestUrl("alice-private"), ExpectedResponse: "invalid workspace request: access to workspace 'alice-private' is forbidden", }, } @@ -212,22 +215,12 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr } }) fakeApp.SignupServiceMock = fake.NewSignupService( - fake.Signup(owner.String(), &signup.Signup{ - Name: "smith2", - APIEndpoint: testServer.URL, - ClusterName: "member-2", - CompliantUsername: "smith2", - Username: "smith2@", - Status: signup.Status{ - Ready: true, - }, - }), - fake.Signup(communityUser.String(), &signup.Signup{ - Name: "communityUser", + fake.Signup(smith.String(), &signup.Signup{ + Name: "smith", APIEndpoint: testServer.URL, ClusterName: "member-2", - CompliantUsername: "communityUser", - Username: "communityUser@", + CompliantUsername: "smith", + Username: "smith@", Status: signup.Status{ Ready: true, }, @@ -255,19 +248,19 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr inf := fake.NewFakeInformer() inf.GetSpaceFunc = func(name string) (*toolchainv1alpha1.Space, error) { switch name { - case "communityspace": - return fake.NewSpace("communityspace", "member-2", "smith2"), nil + case "smith-community": + return fake.NewSpace("smith-community", "member-2", "smith"), nil case "alice-private": return fake.NewSpace("alice-private", "member-2", "alice"), nil } return nil, fmt.Errorf("space not found error") } - sbmycoolSmith2 := fake.NewSpaceBinding("communityspace-smith2", "smith2", "communityspace", "admin") - commSpacePublicViewer := fake.NewSpaceBinding("communityspace-publicviewer", toolchainv1alpha1.KubesawAuthenticatedUsername, "communityspace", "viewer") - alicePrivate := fake.NewSpaceBinding("alice-default", "alice", "alice-default", "admin") + sbSmithCommunitySmith := fake.NewSpaceBinding("smith-community-smith", "smith", "smith-community", "admin") + commSpacePublicViewer := fake.NewSpaceBinding("smith-community-publicviewer", toolchainv1alpha1.KubesawAuthenticatedUsername, "smith-community", "viewer") + alicePrivate := fake.NewSpaceBinding("alice-default", "alice", "alice-private", "admin") - cli := fake.InitClient(s.T(), sbmycoolSmith2, commSpacePublicViewer, alicePrivate) + cli := fake.InitClient(s.T(), sbSmithCommunitySmith, commSpacePublicViewer, alicePrivate) inf.ListSpaceBindingFunc = func(reqs ...labels.Requirement) ([]toolchainv1alpha1.SpaceBinding, error) { sbs := toolchainv1alpha1.SpaceBindingList{} opts := &client.ListOptions{ From e614e382d8effdd19bcef57e9b3d61a69509cb7d Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 12 Sep 2024 10:30:56 +0200 Subject: [PATCH 50/70] rename notReadyUser to john Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_community_test.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index 0d3081e8..156786d5 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -68,11 +68,11 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr s.Run("successfully proxy", func() { smith := uuid.New() alice := uuid.New() - notReadyUser := uuid.New() + john := uuid.New() notSignedUpUser := uuid.New() - httpTestServerResponse := "my response" // Start the member-2 API Server + httpTestServerResponse := "my response" testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") // Set the Access-Control-Allow-Origin header to make sure it's overridden by the proxy response modifier @@ -116,19 +116,19 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr RequestPath: podsRequestUrl("smith-community"), ExpectedResponse: httpTestServerResponse, }, - // Given A not ready user exists - // When the not ready user requests the list of pods in workspace smith-community + // Given The not ready user john exists + // When john requests the list of pods in workspace smith-community // Then the request is forwarded from the proxy - // And the request impersonates the not ready user - // And the request's X-SSO-User Header is set to not ready user's ID + // And the request impersonates john + // And the request's X-SSO-User Header is set to john's ID // And the request is successful "plain http actual request as notReadyUser": { ProxyRequestMethod: "GET", - ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(notReadyUser)}}, + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(john)}}, ExpectedAPIServerRequestHeaders: map[string][]string{ "Authorization": {"Bearer clusterSAToken"}, "Impersonate-User": {toolchainv1alpha1.KubesawAuthenticatedUsername}, - "X-SSO-User": {"username-" + notReadyUser.String()}, + "X-SSO-User": {"username-" + john.String()}, }, ExpectedProxyResponseStatus: http.StatusOK, RequestPath: podsRequestUrl("smith-community"), @@ -235,10 +235,10 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr Ready: true, }, }), - fake.Signup(notReadyUser.String(), &signup.Signup{ - Name: "notReadyUser", - CompliantUsername: "notReadyUser", - Username: "notReadyUser@", + fake.Signup(john.String(), &signup.Signup{ + Name: "john", + CompliantUsername: "john", + Username: "john@", Status: signup.Status{ Ready: false, }, From bd91b58a7383f27eb206af7c286b9d555dfe8274 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 12 Sep 2024 10:34:27 +0200 Subject: [PATCH 51/70] rename notSignedUpUser to bob Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_community_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index 156786d5..2a40cb72 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -68,8 +68,8 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr s.Run("successfully proxy", func() { smith := uuid.New() alice := uuid.New() + bob := uuid.New() john := uuid.New() - notSignedUpUser := uuid.New() // Start the member-2 API Server httpTestServerResponse := "my response" @@ -134,19 +134,19 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr RequestPath: podsRequestUrl("smith-community"), ExpectedResponse: httpTestServerResponse, }, - // Given A not signed up user exists - // When the not signed up user requests the list of pods in workspace smith-community + // Given The not signed up user bob exists + // When bob requests the list of pods in workspace smith-community // Then the request is forwarded from the proxy - // And the request impersonates the not signed up user - // And the request's X-SSO-User Header is set to not signed up user's ID + // And the request impersonates bob + // And the request's X-SSO-User Header is set to bob's ID // And the request is successful - "plain http actual request as notSignedUpUser": { + "plain http actual request as bob": { ProxyRequestMethod: "GET", - ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(notSignedUpUser)}}, + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(bob)}}, ExpectedAPIServerRequestHeaders: map[string][]string{ "Authorization": {"Bearer clusterSAToken"}, "Impersonate-User": {toolchainv1alpha1.KubesawAuthenticatedUsername}, - "X-SSO-User": {"username-" + notSignedUpUser.String()}, + "X-SSO-User": {"username-" + bob.String()}, }, ExpectedProxyResponseStatus: http.StatusOK, RequestPath: podsRequestUrl("smith-community"), From 241fd5a39d1683fc36298ba33af61dee5e38d926 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 12 Sep 2024 10:42:07 +0200 Subject: [PATCH 52/70] remove communityuser in favor of alice Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_community_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index 2a40cb72..ddc1a2ed 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -154,12 +154,12 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr }, // Given smith owns a workspace named smith-community // And smith-community is publicly visible (shared with PublicViewer) - // And a user named communityuser exists - // And smith's smith-community is not directly shared with communityuser - // When communityuser requests the list of pods in workspace smith-community + // And a user named alice exists + // And smith's smith-community is not directly shared with alice + // When alice requests the list of pods in workspace smith-community // Then the request is forwarded from the proxy // And the request impersonates the PublicViewer - // And the request's X-SSO-User Header is set to communityuser's ID + // And the request's X-SSO-User Header is set to alice's ID // And the request is successful "plain http actual request as community user": { ProxyRequestMethod: "GET", From 139809a1bb1715ed5ab323cd149201ca292ff2a4 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 12 Sep 2024 11:08:10 +0200 Subject: [PATCH 53/70] fix linter complaints Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_community_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index ddc1a2ed..b3b7fc63 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -92,7 +92,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr ExpectedResponse string } - podsRequestUrl := func(workspace string) string { + podsRequestURL := func(workspace string) string { return fmt.Sprintf("http://localhost:%s/workspaces/%s/api/pods", port, workspace) } @@ -113,7 +113,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr "X-SSO-User": {"username-" + smith.String()}, }, ExpectedProxyResponseStatus: http.StatusOK, - RequestPath: podsRequestUrl("smith-community"), + RequestPath: podsRequestURL("smith-community"), ExpectedResponse: httpTestServerResponse, }, // Given The not ready user john exists @@ -131,7 +131,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr "X-SSO-User": {"username-" + john.String()}, }, ExpectedProxyResponseStatus: http.StatusOK, - RequestPath: podsRequestUrl("smith-community"), + RequestPath: podsRequestURL("smith-community"), ExpectedResponse: httpTestServerResponse, }, // Given The not signed up user bob exists @@ -149,7 +149,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr "X-SSO-User": {"username-" + bob.String()}, }, ExpectedProxyResponseStatus: http.StatusOK, - RequestPath: podsRequestUrl("smith-community"), + RequestPath: podsRequestURL("smith-community"), ExpectedResponse: httpTestServerResponse, }, // Given smith owns a workspace named smith-community @@ -170,7 +170,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr "X-SSO-User": {"username-" + alice.String()}, }, ExpectedProxyResponseStatus: http.StatusOK, - RequestPath: podsRequestUrl("smith-community"), + RequestPath: podsRequestURL("smith-community"), ExpectedResponse: httpTestServerResponse, }, // Given user alice exists @@ -189,7 +189,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr "X-SSO-User": {"username-" + smith.String()}, }, ExpectedProxyResponseStatus: http.StatusForbidden, - RequestPath: podsRequestUrl("alice-private"), + RequestPath: podsRequestURL("alice-private"), ExpectedResponse: "invalid workspace request: access to workspace 'alice-private' is forbidden", }, } From ea515a6624b58a1592940b2419b99abdf96f63d8 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 12 Sep 2024 18:28:34 +0200 Subject: [PATCH 54/70] Add tests for banneduser in community tests Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_community_test.go | 57 +++++++++++++++++++++++++++++-- 1 file changed, 55 insertions(+), 2 deletions(-) diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index 03e34a38..63e82c6c 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -13,6 +13,7 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/proxy/handlers" "github.com/codeready-toolchain/registration-service/pkg/signup" "github.com/codeready-toolchain/registration-service/test/fake" + authsupport "github.com/codeready-toolchain/toolchain-common/pkg/test/auth" "github.com/google/uuid" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" @@ -20,6 +21,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -70,6 +72,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr alice := uuid.New() bob := uuid.New() john := uuid.New() + eve, eveEmail := uuid.New(), "eve@somecorp.com" // Start the member-2 API Server httpTestServerResponse := "my response" @@ -192,6 +195,33 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr RequestPath: podsRequestURL("alice-private"), ExpectedResponse: "invalid workspace request: access to workspace 'alice-private' is forbidden", }, + // Given banned user eve exists + // And user smith exists + // And smith owns a workspace named smith-community + // And smith-community is publicly visible (shared with PublicViewer) + // When eve requests the list of pods in smith's workspace + // Then the proxy does NOT forward the request + // And the proxy rejects the call with 403 Forbidden + "plain http actual request as banned user to community workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(eve, authsupport.WithEmailClaim(eveEmail))}}, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: podsRequestURL("smith-community"), + ExpectedResponse: "user access is forbidden: user access is forbidden", + }, + // Given banned user eve exists + // And user alice exists + // And alice owns a private workspace + // When eve requests the list of pods in alice's workspace + // Then the proxy does NOT forward the request + // And the proxy rejects the call with 403 Forbidden + "plain http actual request as banned user to private workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(eve, authsupport.WithEmailClaim(eveEmail))}}, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: podsRequestURL("alice-private"), + ExpectedResponse: "user access is forbidden: user access is forbidden", + }, } for k, tc := range tests { @@ -243,6 +273,15 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr Ready: false, }, }), + fake.Signup(eve.String(), &signup.Signup{ + Name: "eve", + CompliantUsername: "eve", + Username: "eve@", + Status: signup.Status{ + Ready: false, + Reason: toolchainv1alpha1.UserSignupUserBannedReason, + }, + }), ) s.Application.MockSignupService(fakeApp.SignupServiceMock) inf := fake.NewFakeInformer() @@ -279,9 +318,23 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr inf.GetNSTemplateTierFunc = func(_ string) (*toolchainv1alpha1.NSTemplateTier, error) { return fake.NewBase1NSTemplateTier(), nil } - inf.ListBannedUsersByEmailFunc = func(_ string) ([]toolchainv1alpha1.BannedUser, error) { - return nil, nil + inf.ListBannedUsersByEmailFunc = func(email string) ([]toolchainv1alpha1.BannedUser, error) { + switch email { + case eveEmail: + return []toolchainv1alpha1.BannedUser{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "eve", + Namespace: "toolchain-host-operator", + }, + Spec: toolchainv1alpha1.BannedUserSpec{}, + }, + }, nil + default: + return nil, nil + } } + s.Application.MockInformerService(inf) fakeApp.MemberClusterServiceMock = s.newMemberClusterServiceWithMembers(testServer.URL) fakeApp.InformerServiceMock = inf From 3db612855f957ef6bf92627ddcacee3a597a8e05 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 12 Sep 2024 18:38:38 +0200 Subject: [PATCH 55/70] use ElementsMatch instead of Equal to reduce flakyness Signed-off-by: Francesco Ilario --- pkg/informers/service/informer_service_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/informers/service/informer_service_test.go b/pkg/informers/service/informer_service_test.go index bbb15b5c..a23c8722 100644 --- a/pkg/informers/service/informer_service_test.go +++ b/pkg/informers/service/informer_service_test.go @@ -488,7 +488,7 @@ func (s *TestInformerServiceSuite) TestInformerService() { require.NotNil(s.T(), rbb) require.NoError(s.T(), err) require.Len(s.T(), rbb, 2, "expected 2 results for email %s", expected[0].Spec.Email) - require.Equal(s.T(), expected, rbb) + require.ElementsMatch(s.T(), expected, rbb) }) }) } From b18db564b0b303a54d4c380858576c1f5e59ef92 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Fri, 13 Sep 2024 13:03:15 +0200 Subject: [PATCH 56/70] remove unnecessary TODO comment Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index c81d6110..9575fba2 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -393,7 +393,6 @@ func (p *Proxy) checkUserIsProvisioned(ctx echo.Context, userID, username string // if the UserSignup is nil or has NOT the CompliantUsername set, // it means that MUR was NOT created and useraccount is NOT provisioned yet - // TODO(@filariow): should we also check if the user is Banned? if userSignup == nil || userSignup.CompliantUsername == "" { cause := errs.New("user is not provisioned (yet)") log.Error(nil, cause, fmt.Sprintf("signup object: %+v", userSignup)) From d5c15036812a2c789cbb7e75ac22237cdf9a7159 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Fri, 13 Sep 2024 13:13:54 +0200 Subject: [PATCH 57/70] refactor tests Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_community_test.go | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index 63e82c6c..385309e2 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -76,15 +76,6 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr // Start the member-2 API Server httpTestServerResponse := "my response" - testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("Content-Type", "application/json") - // Set the Access-Control-Allow-Origin header to make sure it's overridden by the proxy response modifier - w.Header().Set("Access-Control-Allow-Origin", "dummy") - w.WriteHeader(http.StatusOK) - _, err := w.Write([]byte(httpTestServerResponse)) - require.NoError(s.T(), err) - })) - defer testServer.Close() type testCase struct { ProxyRequestMethod string @@ -230,7 +221,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr // given fakeApp.Err = nil - testServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") // Set the Access-Control-Allow-Origin header to make sure it's overridden by the proxy response modifier w.Header().Set("Access-Control-Allow-Origin", "dummy") @@ -243,7 +234,9 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr assert.Equal(s.T(), hv[i], r.Header.Values(hk)[i], "header %s", hk) } } - }) + })) + defer testServer.Close() + fakeApp.SignupServiceMock = fake.NewSignupService( fake.Signup(smith.String(), &signup.Signup{ Name: "smith", From 94e45a03b5b980db97572c7538eca8d1efd41880 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Fri, 13 Sep 2024 13:40:19 +0200 Subject: [PATCH 58/70] refactor tests Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_community_test.go | 239 +++++++++++++++--------------- 1 file changed, 123 insertions(+), 116 deletions(-) diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index 385309e2..8f04859b 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -67,6 +67,10 @@ func (s *TestProxySuite) TestProxyCommunityEnabled() { } func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Proxy, port string) { + podsRequestURL := func(workspace string) string { + return fmt.Sprintf("http://localhost:%s/workspaces/%s/api/pods", port, workspace) + } + s.Run("successfully proxy", func() { smith := uuid.New() alice := uuid.New() @@ -76,21 +80,130 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr // Start the member-2 API Server httpTestServerResponse := "my response" + testServer := httptest.NewServer(nil) + defer testServer.Close() + + // initialize SignupService + signupService := fake.NewSignupService( + fake.Signup(smith.String(), &signup.Signup{ + Name: "smith", + APIEndpoint: testServer.URL, + ClusterName: "member-2", + CompliantUsername: "smith", + Username: "smith@", + Status: signup.Status{ + Ready: true, + }, + }), + fake.Signup(alice.String(), &signup.Signup{ + Name: "alice", + APIEndpoint: testServer.URL, + ClusterName: "member-2", + CompliantUsername: "alice", + Username: "alice@", + Status: signup.Status{ + Ready: true, + }, + }), + fake.Signup(john.String(), &signup.Signup{ + Name: "john", + CompliantUsername: "john", + Username: "john@", + Status: signup.Status{ + Ready: false, + }, + }), + fake.Signup(eve.String(), &signup.Signup{ + Name: "eve", + CompliantUsername: "eve", + Username: "eve@", + Status: signup.Status{ + Ready: false, + Reason: toolchainv1alpha1.UserSignupUserBannedReason, + }, + }), + ) + + // init fakeClient + sbSmithCommunitySmith := fake.NewSpaceBinding("smith-community-smith", "smith", "smith-community", "admin") + commSpacePublicViewer := fake.NewSpaceBinding("smith-community-publicviewer", toolchainv1alpha1.KubesawAuthenticatedUsername, "smith-community", "viewer") + alicePrivate := fake.NewSpaceBinding("alice-default", "alice", "alice-private", "admin") + cli := fake.InitClient(s.T(), sbSmithCommunitySmith, commSpacePublicViewer, alicePrivate) - type testCase struct { + // configure informer + inf := fake.NewFakeInformer() + inf.GetSpaceFunc = func(name string) (*toolchainv1alpha1.Space, error) { + switch name { + case "smith-community": + return fake.NewSpace("smith-community", "member-2", "smith"), nil + case "alice-private": + return fake.NewSpace("alice-private", "member-2", "alice"), nil + } + return nil, fmt.Errorf("space not found error") + } + inf.ListSpaceBindingFunc = func(reqs ...labels.Requirement) ([]toolchainv1alpha1.SpaceBinding, error) { + sbs := toolchainv1alpha1.SpaceBindingList{} + opts := &client.ListOptions{ + LabelSelector: labels.NewSelector().Add(reqs...), + } + log.Printf("received reqs: %v", reqs) + if err := cli.Client.List(context.TODO(), &sbs, opts); err != nil { + return nil, err + } + log.Printf("returning sbs: %v", sbs.Items) + return sbs.Items, nil + } + inf.GetProxyPluginConfigFunc = func(_ string) (*toolchainv1alpha1.ProxyPlugin, error) { + return nil, fmt.Errorf("proxy plugin not found") + } + inf.GetNSTemplateTierFunc = func(_ string) (*toolchainv1alpha1.NSTemplateTier, error) { + return fake.NewBase1NSTemplateTier(), nil + } + inf.ListBannedUsersByEmailFunc = func(email string) ([]toolchainv1alpha1.BannedUser, error) { + switch email { + case eveEmail: + return []toolchainv1alpha1.BannedUser{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "eve", + Namespace: "toolchain-host-operator", + }, + Spec: toolchainv1alpha1.BannedUserSpec{}, + }, + }, nil + default: + return nil, nil + } + } + + // configure fakeApp + fakeApp.Err = nil + fakeApp.MemberClusterServiceMock = s.newMemberClusterServiceWithMembers(testServer.URL) + fakeApp.SignupServiceMock = signupService + fakeApp.InformerServiceMock = inf + + // configure Application + s.Application.MockSignupService(signupService) + s.Application.MockInformerService(inf) + + // configure proxy + p.spaceLister = &handlers.SpaceLister{ + GetSignupFunc: fakeApp.SignupServiceMock.GetSignupFromInformer, + GetInformerServiceFunc: func() appservice.InformerService { + return inf + }, + ProxyMetrics: p.metrics, + } + + // run test cases + tests := map[string]struct { ProxyRequestMethod string ProxyRequestHeaders http.Header ExpectedAPIServerRequestHeaders http.Header ExpectedProxyResponseStatus int RequestPath string ExpectedResponse string - } - - podsRequestURL := func(workspace string) string { - return fmt.Sprintf("http://localhost:%s/workspaces/%s/api/pods", port, workspace) - } - - tests := map[string]testCase{ + }{ // Given smith owns a workspace named smith-community // And smith-community is publicly visible (shared with PublicViewer) // When smith requests the list of pods in workspace smith-community @@ -219,9 +332,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr s.Run(k, func() { // given - fakeApp.Err = nil - - testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + testServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") // Set the Access-Control-Allow-Origin header to make sure it's overridden by the proxy response modifier w.Header().Set("Access-Control-Allow-Origin", "dummy") @@ -234,111 +345,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr assert.Equal(s.T(), hv[i], r.Header.Values(hk)[i], "header %s", hk) } } - })) - defer testServer.Close() - - fakeApp.SignupServiceMock = fake.NewSignupService( - fake.Signup(smith.String(), &signup.Signup{ - Name: "smith", - APIEndpoint: testServer.URL, - ClusterName: "member-2", - CompliantUsername: "smith", - Username: "smith@", - Status: signup.Status{ - Ready: true, - }, - }), - fake.Signup(alice.String(), &signup.Signup{ - Name: "alice", - APIEndpoint: testServer.URL, - ClusterName: "member-2", - CompliantUsername: "alice", - Username: "alice@", - Status: signup.Status{ - Ready: true, - }, - }), - fake.Signup(john.String(), &signup.Signup{ - Name: "john", - CompliantUsername: "john", - Username: "john@", - Status: signup.Status{ - Ready: false, - }, - }), - fake.Signup(eve.String(), &signup.Signup{ - Name: "eve", - CompliantUsername: "eve", - Username: "eve@", - Status: signup.Status{ - Ready: false, - Reason: toolchainv1alpha1.UserSignupUserBannedReason, - }, - }), - ) - s.Application.MockSignupService(fakeApp.SignupServiceMock) - inf := fake.NewFakeInformer() - inf.GetSpaceFunc = func(name string) (*toolchainv1alpha1.Space, error) { - switch name { - case "smith-community": - return fake.NewSpace("smith-community", "member-2", "smith"), nil - case "alice-private": - return fake.NewSpace("alice-private", "member-2", "alice"), nil - } - return nil, fmt.Errorf("space not found error") - } - - sbSmithCommunitySmith := fake.NewSpaceBinding("smith-community-smith", "smith", "smith-community", "admin") - commSpacePublicViewer := fake.NewSpaceBinding("smith-community-publicviewer", toolchainv1alpha1.KubesawAuthenticatedUsername, "smith-community", "viewer") - alicePrivate := fake.NewSpaceBinding("alice-default", "alice", "alice-private", "admin") - - cli := fake.InitClient(s.T(), sbSmithCommunitySmith, commSpacePublicViewer, alicePrivate) - inf.ListSpaceBindingFunc = func(reqs ...labels.Requirement) ([]toolchainv1alpha1.SpaceBinding, error) { - sbs := toolchainv1alpha1.SpaceBindingList{} - opts := &client.ListOptions{ - LabelSelector: labels.NewSelector().Add(reqs...), - } - log.Printf("received reqs: %v", reqs) - if err := cli.Client.List(context.TODO(), &sbs, opts); err != nil { - return nil, err - } - log.Printf("returning sbs: %v", sbs.Items) - return sbs.Items, nil - } - inf.GetProxyPluginConfigFunc = func(_ string) (*toolchainv1alpha1.ProxyPlugin, error) { - return nil, fmt.Errorf("proxy plugin not found") - } - inf.GetNSTemplateTierFunc = func(_ string) (*toolchainv1alpha1.NSTemplateTier, error) { - return fake.NewBase1NSTemplateTier(), nil - } - inf.ListBannedUsersByEmailFunc = func(email string) ([]toolchainv1alpha1.BannedUser, error) { - switch email { - case eveEmail: - return []toolchainv1alpha1.BannedUser{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "eve", - Namespace: "toolchain-host-operator", - }, - Spec: toolchainv1alpha1.BannedUserSpec{}, - }, - }, nil - default: - return nil, nil - } - } - - s.Application.MockInformerService(inf) - fakeApp.MemberClusterServiceMock = s.newMemberClusterServiceWithMembers(testServer.URL) - fakeApp.InformerServiceMock = inf - - p.spaceLister = &handlers.SpaceLister{ - GetSignupFunc: fakeApp.SignupServiceMock.GetSignupFromInformer, - GetInformerServiceFunc: func() appservice.InformerService { - return inf - }, - ProxyMetrics: p.metrics, - } + }) // prepare request req, err := http.NewRequest(tc.ProxyRequestMethod, tc.RequestPath, nil) From 306b6d61462ede322bad5742ae2f7b84df6d3607 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Fri, 13 Sep 2024 18:06:38 +0200 Subject: [PATCH 59/70] revert unneeded changes to test/fake/proxy.go Signed-off-by: Francesco Ilario --- test/fake/proxy.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/test/fake/proxy.go b/test/fake/proxy.go index 22d350cd..5dc68736 100644 --- a/test/fake/proxy.go +++ b/test/fake/proxy.go @@ -21,11 +21,10 @@ type ProxyFakeApp struct { } func (a *ProxyFakeApp) InformerService() service.InformerService { - if a.InformerServiceMock == nil { - panic("InformerService shouldn't be called") + if a.InformerServiceMock != nil { + return a.InformerServiceMock } - - return a.InformerServiceMock + panic("InformerService shouldn't be called") } func (a *ProxyFakeApp) SignupService() service.SignupService { From 13d335a2b9fb0581a8cb124ac1e14c29313789d5 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 23 Sep 2024 18:56:04 +0200 Subject: [PATCH 60/70] check proxy has forwarded the call Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_community_test.go | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index 8f04859b..e3fe513f 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -15,6 +15,7 @@ import ( "github.com/codeready-toolchain/registration-service/test/fake" authsupport "github.com/codeready-toolchain/toolchain-common/pkg/test/auth" "github.com/google/uuid" + "go.uber.org/atomic" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" testconfig "github.com/codeready-toolchain/toolchain-common/pkg/test/config" @@ -283,18 +284,11 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr // Given user alice exists // And alice owns a private workspace // When smith requests the list of pods in alice's workspace - // Then the request is forwarded from the proxy - // And the request impersonates smith - // And the request's X-SSO-User Header is set to smith's ID - // And the request is NOT successful + // Then the proxy does NOT forward the request + // And the proxy rejects the call with 403 Forbidden "plain http actual request as non-owner to private workspace": { - ProxyRequestMethod: "GET", - ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(smith)}}, - ExpectedAPIServerRequestHeaders: map[string][]string{ - "Authorization": {"Bearer clusterSAToken"}, - "Impersonate-User": {"smith"}, - "X-SSO-User": {"username-" + smith.String()}, - }, + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(smith)}}, ExpectedProxyResponseStatus: http.StatusForbidden, RequestPath: podsRequestURL("alice-private"), ExpectedResponse: "invalid workspace request: access to workspace 'alice-private' is forbidden", @@ -330,9 +324,13 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr for k, tc := range tests { s.Run(k, func() { + testServerInvoked := atomic.NewBool(false) // given testServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + v := testServerInvoked.Swap(true) + require.False(s.T(), v, "expected handler to be invoked just one time") + w.Header().Set("Content-Type", "application/json") // Set the Access-Control-Allow-Origin header to make sure it's overridden by the proxy response modifier w.Header().Set("Access-Control-Allow-Origin", "dummy") @@ -368,6 +366,13 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr defer resp.Body.Close() assert.Equal(s.T(), tc.ExpectedProxyResponseStatus, resp.StatusCode) s.assertResponseBody(resp, tc.ExpectedResponse) + + forwardExpected := len(tc.ExpectedAPIServerRequestHeaders) > 0 + requestForwarded := testServerInvoked.Load() + require.True(s.T(), + forwardExpected == requestForwarded, + "expecting call forward to be %v, got %v", forwardExpected, requestForwarded, + ) }) } }) From 15fa0cd3124749586210b00e5cb34e5f85741b2d Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 23 Sep 2024 18:57:34 +0200 Subject: [PATCH 61/70] add comments and fix typos Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_community_test.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index e3fe513f..719bbf68 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -73,10 +73,15 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr } s.Run("successfully proxy", func() { + // user with public workspace smith := uuid.New() + // user with private workspace alice := uuid.New() + // unsigned user bob := uuid.New() + // not ready john := uuid.New() + // banned user eve, eveEmail := uuid.New(), "eve@somecorp.com" // Start the member-2 API Server @@ -248,7 +253,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr // And the request impersonates bob // And the request's X-SSO-User Header is set to bob's ID // And the request is successful - "plain http actual request as bob": { + "plain http actual request as not signed up user": { ProxyRequestMethod: "GET", ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(bob)}}, ExpectedAPIServerRequestHeaders: map[string][]string{ @@ -307,7 +312,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr RequestPath: podsRequestURL("smith-community"), ExpectedResponse: "user access is forbidden: user access is forbidden", }, - // Given banned user eve exists + // Given banned user eve exist // And user alice exists // And alice owns a private workspace // When eve requests the list of pods in alice's workspace @@ -335,7 +340,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr // Set the Access-Control-Allow-Origin header to make sure it's overridden by the proxy response modifier w.Header().Set("Access-Control-Allow-Origin", "dummy") w.WriteHeader(http.StatusOK) - _, err := w.Write([]byte("my response")) + _, err := w.Write([]byte(httpTestServerResponse)) require.NoError(s.T(), err) for hk, hv := range tc.ExpectedAPIServerRequestHeaders { require.Len(s.T(), r.Header.Values(hk), len(hv)) From 83ea573f94748d7f9106885ffca47bd3e208e27a Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 23 Sep 2024 18:57:49 +0200 Subject: [PATCH 62/70] add more test cases Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_community_test.go | 82 +++++++++++++++++++++++++++++++ pkg/proxy/proxy_test.go | 26 +++++++++- 2 files changed, 107 insertions(+), 1 deletion(-) diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index 719bbf68..97b88d39 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -72,6 +72,10 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr return fmt.Sprintf("http://localhost:%s/workspaces/%s/api/pods", port, workspace) } + podsInNamespaceRequestURL := func(workspace string, namespace string) string { + return fmt.Sprintf("http://localhost:%s/workspaces/%s/api/namespaces/%s/pods", port, workspace, namespace) + } + s.Run("successfully proxy", func() { // user with public workspace smith := uuid.New() @@ -325,6 +329,84 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr RequestPath: podsRequestURL("alice-private"), ExpectedResponse: "user access is forbidden: user access is forbidden", }, + // Given user alice exists + // And alice owns a private workspace + // When alice requests the list of pods in a non existing namespace in alice's workspace + // Then the proxy does NOT forward the request + // And the proxy rejects the call with 403 Forbidden + "plain http request as owner to not existing namespace in private workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(alice)}}, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: podsInNamespaceRequestURL("alice-private", "not-existing"), + ExpectedResponse: "invalid workspace request: access to namespace 'not-existing' in workspace 'alice-private' is forbidden", + }, + // Given smith owns a workspace named smith-community + // And smith-community is publicly visible (shared with PublicViewer) + // When smith requests the list of pods in a non existing namespace in workspace smith-community + // Then the request is forwarded from the proxy + // And the request impersonates smith + // And the request's X-SSO-User Header is set to smith's ID + // And the request is successful + "plain http request as owner to not existing namespace in community workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(smith)}}, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: podsInNamespaceRequestURL("smith-community", "not-existing"), + ExpectedResponse: "invalid workspace request: access to namespace 'not-existing' in workspace 'smith-community' is forbidden", + }, + // Given smith owns a workspace named smith-community + // And smith-community is publicly visible (shared with PublicViewer) + // And user alice exists + // When alice requests the list of pods in a non existing namespace in smith's workspace + // Then the proxy does NOT forward the request + // And the proxy rejects the call with 403 Forbidden + "plain http request as community user to not existing namespace in community workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(alice)}}, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: podsInNamespaceRequestURL("smith-community", "not-existing"), + ExpectedResponse: "invalid workspace request: access to namespace 'not-existing' in workspace 'smith-community' is forbidden", + }, + // Given smith owns a workspace named smith-community + // And smith-community is publicly visible (shared with PublicViewer) + // When bob requests the list of pods in a non existing namespace in smith's workspace + // Then the proxy does NOT forward the request + // And the proxy rejects the call with 403 Forbidden + "plain http request as unsigned user to not existing namespace in community workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(bob)}}, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: podsInNamespaceRequestURL("smith-community", "not-existing"), + ExpectedResponse: "invalid workspace request: access to namespace 'not-existing' in workspace 'smith-community' is forbidden", + }, + // Given smith owns a workspace named smith-community + // And smith-community is publicly visible (shared with PublicViewer) + // And not ready user john exists + // When john requests the list of pods in a non existing namespace in smith's workspace + // Then the proxy does NOT forward the request + // And the proxy rejects the call with 403 Forbidden + "plain http request as notReadyUser to not existing namespace in community workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(john)}}, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: podsInNamespaceRequestURL("smith-community", "not-existing"), + ExpectedResponse: "invalid workspace request: access to namespace 'not-existing' in workspace 'smith-community' is forbidden", + }, + // Given banned user eve exists + // And user smith exists + // And smith owns a workspace named smith-community + // And smith-community is publicly visible (shared with PublicViewer) + // When eve requests the list of pods in a non existing namespace smith's workspace + // Then the proxy does NOT forward the request + // And the proxy rejects the call with 403 Forbidden + "plain http actual request as banned user to not existing namespace community workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(eve, authsupport.WithEmailClaim(eveEmail))}}, + ExpectedProxyResponseStatus: http.StatusForbidden, + RequestPath: podsInNamespaceRequestURL("smith-community", "not-existing"), + ExpectedResponse: "user access is forbidden: user access is forbidden", + }, } for k, tc := range tests { diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index 9c4f0f02..3b02406a 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -678,7 +678,7 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy, publ }, "unauthorized if workspace not exists": { ProxyRequestPaths: map[string]string{ - "not-existing-workspace-namespace": "http://localhost:8081/workspaces/not-existing-workspace/api/namespaces/not-existing-namespace/pods", + "not existing workspace namespace": "http://localhost:8081/workspaces/not-existing-workspace/api/namespaces/not-existing-namespace/pods", }, ProxyRequestMethod: "GET", ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(userID)}}, @@ -688,6 +688,30 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy, publ ExpectedResponse: ptr("unable to get target cluster: access to workspace 'not-existing-workspace' is forbidden"), ExpectedProxyResponseStatus: http.StatusInternalServerError, }, + "unauthorized if namespace does not exist in implicit workspace": { + ProxyRequestPaths: map[string]string{ + "not existing namespace": "http://localhost:8081/api/namespaces/not-existing-namespace/pods", + }, + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(userID)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + }, + ExpectedResponse: ptr("invalid workspace request: access to namespace 'not-existing-namespace' in workspace 'mycoolworkspace' is forbidden"), + ExpectedProxyResponseStatus: http.StatusForbidden, + }, + "unauthorized if namespace does not exist in explicit workspace": { + ProxyRequestPaths: map[string]string{ + "not existing namespace": "http://localhost:8081/workspaces/mycoolworkspace/api/namespaces/not-existing-namespace/pods", + }, + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(userID)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + }, + ExpectedResponse: ptr("invalid workspace request: access to namespace 'not-existing-namespace' in workspace 'mycoolworkspace' is forbidden"), + ExpectedProxyResponseStatus: http.StatusForbidden, + }, } rejectedHeaders := []headerToAdd{ From 5d0f60491074a36d21fcee3fed0b9e70e7d8a9f7 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 23 Sep 2024 19:23:32 +0200 Subject: [PATCH 63/70] remove dead code Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 9575fba2..e2e8a8b5 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -431,10 +431,6 @@ func (p *Proxy) getClusterAccessAsUserOrPublicViewer(ctx echo.Context, userID, u // proceed as PublicViewer if the feature is enabled and userSignup is nil publicViewerEnabled := context.IsPublicViewerEnabled(ctx) if publicViewerEnabled && !userHasDirectAccess(userSignup, workspace) { - if !publicViewerHasAccess(workspace) { - return nil, crterrors.NewForbiddenError("invalid workspace request", fmt.Sprintf("access to workspace '%s' is forbidden", workspace.Name)) - } - return p.app.MemberClusterService().GetClusterAccess( toolchainv1alpha1.KubesawAuthenticatedUsername, toolchainv1alpha1.KubesawAuthenticatedUsername, @@ -447,10 +443,6 @@ func (p *Proxy) getClusterAccessAsUserOrPublicViewer(ctx echo.Context, userID, u return p.app.MemberClusterService().GetClusterAccess(userID, username, workspace.Name, proxyPluginName, publicViewerEnabled) } -func publicViewerHasAccess(workspace *toolchainv1alpha1.Workspace) bool { - return userHasBinding(toolchainv1alpha1.KubesawAuthenticatedUsername, workspace) -} - // userHasDirectAccess checks if an UserSignup has access to a workspace. // Workspace's bindings are obtained from its `status.bindings` property. func userHasDirectAccess(signup *signup.Signup, workspace *toolchainv1alpha1.Workspace) bool { From c0712255516d1f08527fc0e124173893be32a947 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 23 Sep 2024 19:23:48 +0200 Subject: [PATCH 64/70] consolidate code Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index e2e8a8b5..7ada33d4 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -422,7 +422,7 @@ func (p *Proxy) getClusterAccess(ctx echo.Context, userID, username, proxyPlugin // this function returns an error. func (p *Proxy) getClusterAccessAsUserOrPublicViewer(ctx echo.Context, userID, username, proxyPluginName string, workspace *toolchainv1alpha1.Workspace) (*access.ClusterAccess, error) { // retrieve the requesting user's UserSignup - userSignup, err := p.spaceLister.GetSignupFunc(nil, userID, username, false) + userSignup, err := p.app.SignupService().GetSignupFromInformer(nil, userID, username, false) if err != nil { log.Error(nil, err, fmt.Sprintf("error retrieving user signup for userID '%s' and username '%s'", userID, username)) return nil, crterrors.NewInternalError(errs.New("unable to get user info"), "error retrieving user") From e09cd75675e68473f010276ab6f688fc24638844 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Mon, 23 Sep 2024 20:06:07 +0200 Subject: [PATCH 65/70] fix linter complaints Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_community_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index 97b88d39..dca393eb 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -456,8 +456,8 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr forwardExpected := len(tc.ExpectedAPIServerRequestHeaders) > 0 requestForwarded := testServerInvoked.Load() - require.True(s.T(), - forwardExpected == requestForwarded, + require.Equal(s.T(), + forwardExpected, requestForwarded, "expecting call forward to be %v, got %v", forwardExpected, requestForwarded, ) }) From 599e2d2424b3fb2f5752ad57e7aef0496fe61783 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Wed, 25 Sep 2024 11:33:56 +0200 Subject: [PATCH 66/70] add comment Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 7ada33d4..9d0713c7 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -683,6 +683,7 @@ func (p *Proxy) newReverseProxy(ctx echo.Context, target *access.ClusterAccess, req := ctx.Request() targetQuery := target.APIURL().RawQuery username, _ := ctx.Get(context.UsernameKey).(string) + // set username in context for logging purposes ctx.Set(context.ImpersonateUser, target.Username()) director := func(req *http.Request) { From 150dbd02baf7e935b66fd56ccb22db1663b41d35 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Wed, 25 Sep 2024 11:45:53 +0200 Subject: [PATCH 67/70] check proxy requires email in the JWT token Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_test.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index 3b02406a..6678d4d3 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -240,6 +240,23 @@ func (s *TestProxySuite) checkPlainHTTPErrors(fakeApp *fake.ProxyFakeApp) { s.assertResponseBody(resp, "invalid bearer token: unable to extract userID from token: token does not comply to expected claims: subject missing") }) + s.Run("unauthorized if can't extract email from a valid token", func() { + // when + req, err := http.NewRequest("GET", "http://localhost:8081/api/mycoolworkspace/pods", nil) + require.NoError(s.T(), err) + require.NotNil(s.T(), req) + userID := uuid.New() + req.Header.Set("Authorization", "Bearer "+s.token(userID, authsupport.WithEmailClaim(""))) + resp, err := http.DefaultClient.Do(req) + + // then + require.NoError(s.T(), err) + require.NotNil(s.T(), resp) + defer resp.Body.Close() + assert.Equal(s.T(), http.StatusUnauthorized, resp.StatusCode) + s.assertResponseBody(resp, "invalid bearer token: unable to extract userID from token: token does not comply to expected claims: email missing") + }) + s.Run("unauthorized if workspace context is invalid", func() { // when req := s.request() From c343167ac1730740b506f7a70ce3c5d119864c37 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Wed, 25 Sep 2024 17:46:42 +0200 Subject: [PATCH 68/70] remove publicViewer leftovers from test Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_test.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index 6678d4d3..1408dd43 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -128,7 +128,7 @@ func (s *TestProxySuite) TestProxy() { s.checkPlainHTTPErrors(fakeApp) s.checkWebsocketsError() s.checkWebLogin() - s.checkProxyOK(fakeApp, p, false) + s.checkProxyOK(fakeApp, p) }) } } @@ -495,7 +495,7 @@ func (s *TestProxySuite) checkWebLogin() { }) } -func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy, publicViewerEnabled bool) { +func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { s.Run("successfully proxy", func() { userID := uuid.New() @@ -845,9 +845,6 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy, publ if req.Values().List()[0] == "smith2" || req.Values().List()[0] == "mycoolworkspace" { spaceBindings = []toolchainv1alpha1.SpaceBinding{*fake.NewSpaceBinding("mycoolworkspace-smith2", "smith2", "mycoolworkspace", "admin")} } - if publicViewerEnabled && req.Values().List()[0] == toolchainv1alpha1.KubesawAuthenticatedUsername { - spaceBindings = []toolchainv1alpha1.SpaceBinding{*fake.NewSpaceBinding("communityspace-publicviewer", "publicviewer", "communityspace", "viewer")} - } } return spaceBindings, nil } From 8ba345269977ae748a863c8f05d341d47f802a8a Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Wed, 25 Sep 2024 19:19:03 +0200 Subject: [PATCH 69/70] sort proxy middlewares Signed-off-by: Francesco Ilario --- pkg/proxy/proxy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 9d0713c7..08ded564 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -119,8 +119,8 @@ func (p *Proxy) StartProxy(port string) *http.Server { return next(ctx) } }, - p.addPublicViewerContext(), p.ensureUserIsNotBanned(), + p.addPublicViewerContext(), ) // middleware after routing From 0ffff03e3acb1e7945c8fa834d98174eb6f51411 Mon Sep 17 00:00:00 2001 From: Francesco Ilario Date: Thu, 26 Sep 2024 09:52:20 +0200 Subject: [PATCH 70/70] remove unneeded comment Signed-off-by: Francesco Ilario --- pkg/proxy/proxy_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index 88e9312f..cb1aa373 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -687,7 +687,6 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { "Authorization": {"Bearer clusterSAToken"}, }, ExpectedProxyResponseStatus: http.StatusInternalServerError, - // ExpectedResponse: "invalid workspace request: access to workspace 'alice-private' is forbidden", OverrideGetSignupFunc: func(_ *gin.Context, _, _ string, _ bool) (*signup.Signup, error) { return nil, fmt.Errorf("test error") },