addressing comments

backend/src/apiserver/server/run_server.go

@@ -19,9 +19,11 @@ import (
 
 	"github.com/golang/protobuf/ptypes/empty"
 	api "github.com/kubeflow/pipelines/backend/api/go_client"
+	"github.com/kubeflow/pipelines/backend/src/apiserver/common"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/model"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/resource"
 	"github.com/kubeflow/pipelines/backend/src/common/util"
+	"github.com/pkg/errors"
 )
 
 type RunServer struct {
@@ -72,7 +74,7 @@ func (s *RunServer) ListRuns(ctx context.Context, request *api.ListRunsRequest)
 }
 
 func (s *RunServer) ArchiveRun(ctx context.Context, request *api.ArchiveRunRequest) (*empty.Empty, error) {
-	err := CanAccessRun(s.resourceManager, ctx, request.Id)
+	err := s.canAccessRun(ctx, request.Id)
 	if err != nil {
 		return nil, util.Wrap(err, "Failed to authorize the requests.")
 	}
@@ -84,7 +86,7 @@ func (s *RunServer) ArchiveRun(ctx context.Context, request *api.ArchiveRunReque
 }
 
 func (s *RunServer) UnarchiveRun(ctx context.Context, request *api.UnarchiveRunRequest) (*empty.Empty, error) {
-	err := CanAccessRun(s.resourceManager, ctx, request.Id)
+	err := s.canAccessRun(ctx, request.Id)
 	if err != nil {
 		return nil, util.Wrap(err, "Failed to authorize the requests.")
 	}
@@ -96,7 +98,7 @@ func (s *RunServer) UnarchiveRun(ctx context.Context, request *api.UnarchiveRunR
 }
 
 func (s *RunServer) DeleteRun(ctx context.Context, request *api.DeleteRunRequest) (*empty.Empty, error) {
-	err := CanAccessRun(s.resourceManager, ctx, request.Id)
+	err := s.canAccessRun(ctx, request.Id)
 	if err != nil {
 		return nil, util.Wrap(err, "Failed to authorize the requests.")
 	}
@@ -155,7 +157,7 @@ func (s *RunServer) validateCreateRunRequest(request *api.CreateRunRequest) erro
 }
 
 func (s *RunServer) TerminateRun(ctx context.Context, request *api.TerminateRunRequest) (*empty.Empty, error) {
-	err := CanAccessRun(s.resourceManager, ctx, request.RunId)
+	err := s.canAccessRun(ctx, request.RunId)
 	if err != nil {
 		return nil, util.Wrap(err, "Failed to authorize the requests.")
 	}
@@ -167,7 +169,7 @@ func (s *RunServer) TerminateRun(ctx context.Context, request *api.TerminateRunR
 }
 
 func (s *RunServer) RetryRun(ctx context.Context, request *api.RetryRunRequest) (*empty.Empty, error) {
-	err := CanAccessRun(s.resourceManager, ctx, request.RunId)
+	err := s.canAccessRun(ctx, request.RunId)
 	if err != nil {
 		return nil, util.Wrap(err, "Failed to authorize the requests.")
 	}
@@ -179,6 +181,26 @@ func (s *RunServer) RetryRun(ctx context.Context, request *api.RetryRunRequest)
 
 }
 
+func (s *RunServer) canAccessRun(ctx context.Context, runId string) error {
+	if common.IsMultiUserMode() == false {
+		// Skip authz if not multi-user mode.
+		return nil
+	}
+	runDetail, err := s.resourceManager.GetRun(runId)
+	if err != nil {
+		return util.Wrap(err, "Failed to authorize with the run Id.")
+	}
+	namespace := model.GetNamespaceFromModelResourceReferences(runDetail.ResourceReferences)
+	if len(namespace) == 0 {
+		return util.NewInternalServerError(errors.New("There is no namespace in the ResourceReferences"), "There is no namespace in the ResourceReferences")
+	}
+	err = isAuthorized(s.resourceManager, ctx, namespace)
+	if err != nil {
+		return util.Wrap(err, "Failed to authorize with API resource references")
+	}
+	return nil
+}
+
 func NewRunServer(resourceManager *resource.ResourceManager) *RunServer {
 	return &RunServer{resourceManager: resourceManager}
 }

backend/src/apiserver/server/run_server_test.go

@@ -7,9 +7,12 @@ import (
 	"github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
 	"github.com/golang/protobuf/ptypes/timestamp"
 	api "github.com/kubeflow/pipelines/backend/api/go_client"
+	"github.com/kubeflow/pipelines/backend/src/apiserver/common"
 	"github.com/kubeflow/pipelines/backend/src/common/util"
+	"github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
 	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/metadata"
 )
 
 func TestCreateRun(t *testing.T) {
@@ -318,3 +321,69 @@ func TestReportRunMetrics_PartialFailures(t *testing.T) {
 	}
 	assert.Equal(t, expectedResponse, response)
 }
+
+func TestCanAccessRun_Unauthorized(t *testing.T) {
+	clients, manager, experiment := initWithExperiment_KFAM_Unauthorized(t)
+	defer clients.Close()
+	runServer := RunServer{resourceManager: manager}
+	viper.Set(common.MultiUserMode, "true")
+	md := metadata.New(map[string]string{common.GoogleIAPUserIdentityHeader: "accounts.google.com:user@google.com"})
+	ctx := metadata.NewIncomingContext(context.Background(), md)
+
+	apiRun := &api.Run{
+		Name: "run1",
+		PipelineSpec: &api.PipelineSpec{
+			WorkflowManifest: testWorkflow.ToStringForStore(),
+			Parameters: []*api.Parameter{
+				{Name: "param1", Value: "world"},
+			},
+		},
+		ResourceReferences: []*api.ResourceReference{
+			{
+				Key:          &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns"},
+				Relationship: api.Relationship_OWNER,
+			},
+			{
+				Key:          &api.ResourceKey{Type: api.ResourceType_EXPERIMENT, Id: experiment.UUID},
+				Relationship: api.Relationship_OWNER,
+			},
+		},
+	}
+	runDetail, _ := manager.CreateRun(apiRun)
+
+	err := runServer.canAccessRun(ctx, runDetail.UUID)
+	assert.NotNil(t, err)
+}
+
+func TestCanAccessRun_Authorized(t *testing.T) {
+	clients, manager, experiment := initWithExperiment(t)
+	defer clients.Close()
+	runServer := RunServer{resourceManager: manager}
+	viper.Set(common.MultiUserMode, "true")
+	md := metadata.New(map[string]string{common.GoogleIAPUserIdentityHeader: "accounts.google.com:user@google.com"})
+	ctx := metadata.NewIncomingContext(context.Background(), md)
+
+	apiRun := &api.Run{
+		Name: "run1",
+		PipelineSpec: &api.PipelineSpec{
+			WorkflowManifest: testWorkflow.ToStringForStore(),
+			Parameters: []*api.Parameter{
+				{Name: "param1", Value: "world"},
+			},
+		},
+		ResourceReferences: []*api.ResourceReference{
+			{
+				Key:          &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns"},
+				Relationship: api.Relationship_OWNER,
+			},
+			{
+				Key:          &api.ResourceKey{Type: api.ResourceType_EXPERIMENT, Id: experiment.UUID},
+				Relationship: api.Relationship_OWNER,
+			},
+		},
+	}
+	runDetail, _ := manager.CreateRun(apiRun)
+
+	err := runServer.canAccessRun(ctx, runDetail.UUID)
+	assert.Nil(t, err)
+}

backend/src/apiserver/server/util.go

@@ -17,7 +17,6 @@ import (
 	"github.com/golang/glog"
 	api "github.com/kubeflow/pipelines/backend/api/go_client"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/common"
-	"github.com/kubeflow/pipelines/backend/src/apiserver/model"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/resource"
 	"github.com/kubeflow/pipelines/backend/src/common/util"
 	"github.com/pkg/errors"
@@ -298,26 +297,6 @@ func getUserIdentity(ctx context.Context) (string, error) {
 	return "", util.NewBadRequestError(errors.New("Request header error: there is no user identity header."), "Request header error: there is no user identity header.")
 }
 
-func CanAccessRun(resourceManager *resource.ResourceManager, ctx context.Context, runId string) error {
-	if common.IsMultiUserMode() == false {
-		// Skip authz if not multi-user mode.
-		return nil
-	}
-	runDetail, err := resourceManager.GetRun(runId)
-	if err != nil {
-		return util.Wrap(err, "Failed to authorize with the run Id.")
-	}
-	namespace := model.GetNamespaceFromModelResourceReferences(runDetail.ResourceReferences)
-	if len(namespace) == 0 {
-		return util.NewInternalServerError(errors.New("There is no namespace in the ResourceReferences"), "There is no namespace in the ResourceReferences")
-	}
-	err = isAuthorized(resourceManager, ctx, namespace)
-	if err != nil {
-		return util.Wrap(err, "Failed to authorize with API resource references")
-	}
-	return nil
-}
-
 func CanAccessNamespaceInResourceReferences(resourceManager *resource.ResourceManager, ctx context.Context, resourceRefs []*api.ResourceReference) error {
 	if common.IsMultiUserMode() == false {
 		// Skip authz if not multi-user mode.

backend/src/apiserver/server/util_test.go

@@ -362,38 +362,6 @@ func TestCanAccessNamespaceInResourceReferencesUnauthorized(t *testing.T) {
 	assert.NotNil(t, err)
 }
 
-func TestCanAccessRun_Unauthorized(t *testing.T) {
-	clients, manager, experiment := initWithExperiment_KFAM_Unauthorized(t)
-	defer clients.Close()
-	viper.Set(common.MultiUserMode, "true")
-	md := metadata.New(map[string]string{common.GoogleIAPUserIdentityHeader: "accounts.google.com:user@google.com"})
-	ctx := metadata.NewIncomingContext(context.Background(), md)
-
-	apiRun := &api.Run{
-		Name: "run1",
-		PipelineSpec: &api.PipelineSpec{
-			WorkflowManifest: testWorkflow.ToStringForStore(),
-			Parameters: []*api.Parameter{
-				{Name: "param1", Value: "world"},
-			},
-		},
-		ResourceReferences: []*api.ResourceReference{
-			{
-				Key:          &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns"},
-				Relationship: api.Relationship_OWNER,
-			},
-			{
-				Key:          &api.ResourceKey{Type: api.ResourceType_EXPERIMENT, Id: experiment.UUID},
-				Relationship: api.Relationship_OWNER,
-			},
-		},
-	}
-	runDetail, _ := manager.CreateRun(apiRun)
-
-	err := CanAccessRun(manager, ctx, runDetail.UUID)
-	assert.NotNil(t, err)
-}
-
 func TestCanAccessNamespaceInResourceReferences_Authorized(t *testing.T) {
 	clients, manager, _ := initWithExperiment(t)
 	defer clients.Close()
@@ -410,35 +378,3 @@ func TestCanAccessNamespaceInResourceReferences_Authorized(t *testing.T) {
 	err := CanAccessNamespaceInResourceReferences(manager, ctx, references)
 	assert.Nil(t, err)
 }
-
-func TestCanAccessRun_Authorized(t *testing.T) {
-	clients, manager, experiment := initWithExperiment(t)
-	defer clients.Close()
-	viper.Set(common.MultiUserMode, "true")
-	md := metadata.New(map[string]string{common.GoogleIAPUserIdentityHeader: "accounts.google.com:user@google.com"})
-	ctx := metadata.NewIncomingContext(context.Background(), md)
-
-	apiRun := &api.Run{
-		Name: "run1",
-		PipelineSpec: &api.PipelineSpec{
-			WorkflowManifest: testWorkflow.ToStringForStore(),
-			Parameters: []*api.Parameter{
-				{Name: "param1", Value: "world"},
-			},
-		},
-		ResourceReferences: []*api.ResourceReference{
-			{
-				Key:          &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns"},
-				Relationship: api.Relationship_OWNER,
-			},
-			{
-				Key:          &api.ResourceKey{Type: api.ResourceType_EXPERIMENT, Id: experiment.UUID},
-				Relationship: api.Relationship_OWNER,
-			},
-		},
-	}
-	runDetail, _ := manager.CreateRun(apiRun)
-
-	err := CanAccessRun(manager, ctx, runDetail.UUID)
-	assert.Nil(t, err)
-}