From b70e7ff54f2cfa4c0c14c312299ccb65cc96f5f5 Mon Sep 17 00:00:00 2001
From: Chen Sun <chensun@users.noreply.github.com>
Date: Fri, 28 Feb 2020 17:57:45 -0800
Subject: [PATCH 1/6] Experiment API implementation

---
 .../src/apiserver/resource/model_converter.go |  18 ++
 .../apiserver/resource/resource_manager.go    |  20 +-
 .../resource/resource_manager_test.go         |  16 +-
 backend/src/apiserver/server/api_converter.go |  24 +-
 .../src/apiserver/server/experiment_server.go |  64 ++++-
 .../server/experiment_server_test.go          | 238 +++++++++++++++++-
 .../src/apiserver/server/run_server_test.go   |  10 +-
 backend/src/apiserver/server/test_util.go     |  34 ++-
 backend/src/apiserver/server/util_test.go     |  10 +-
 .../src/apiserver/storage/experiment_store.go |  17 +-
 .../storage/experiment_store_test.go          |  17 +-
 11 files changed, 419 insertions(+), 49 deletions(-)

diff --git a/backend/src/apiserver/resource/model_converter.go b/backend/src/apiserver/resource/model_converter.go
index 630c2bc21f2..fd3a3e615f5 100644
--- a/backend/src/apiserver/resource/model_converter.go
+++ b/backend/src/apiserver/resource/model_converter.go
@@ -24,6 +24,24 @@ import (
 	"github.com/kubeflow/pipelines/backend/src/common/util"
 )
 
+func (r *ResourceManager) ToModelExperiment(apiExperiment *api.Experiment) (*model.Experiment, error) {
+	namespace := ""
+	if common.IsMultiUserMode() {
+		resourceReferences := apiExperiment.GetResourceReferences()
+		if len(resourceReferences) != 1 ||
+			resourceReferences[0].Key.Type != api.ResourceType_NAMESPACE ||
+			resourceReferences[0].Relationship != api.Relationship_OWNER {
+			return nil, util.NewInvalidInputError("Invalid resource references for experiment: %v", resourceReferences)
+		}
+		namespace = resourceReferences[0].Key.Id
+	}
+	return &model.Experiment{
+		Name:        apiExperiment.Name,
+		Description: apiExperiment.Description,
+		Namespace:   namespace,
+	}, nil
+}
+
 func (r *ResourceManager) ToModelRunMetric(metric *api.RunMetric, runUUID string) *model.RunMetric {
 	return &model.RunMetric{
 		RunUUID:     runUUID,
diff --git a/backend/src/apiserver/resource/resource_manager.go b/backend/src/apiserver/resource/resource_manager.go
index a3ed4101326..51390913662 100644
--- a/backend/src/apiserver/resource/resource_manager.go
+++ b/backend/src/apiserver/resource/resource_manager.go
@@ -108,7 +108,11 @@ func (r *ResourceManager) GetTime() util.TimeInterface {
 	return r.time
 }
 
-func (r *ResourceManager) CreateExperiment(experiment *model.Experiment) (*model.Experiment, error) {
+func (r *ResourceManager) CreateExperiment(apiExperiment *api.Experiment) (*model.Experiment, error) {
+	experiment, err := r.ToModelExperiment(apiExperiment)
+	if err != nil {
+		return nil, util.Wrap(err, "Failed to convert experiment model")
+	}
 	return r.experimentStore.CreateExperiment(experiment)
 }
 
@@ -116,9 +120,9 @@ func (r *ResourceManager) GetExperiment(experimentId string) (*model.Experiment,
 	return r.experimentStore.GetExperiment(experimentId)
 }
 
-func (r *ResourceManager) ListExperiments(opts *list.Options) (
+func (r *ResourceManager) ListExperiments(filterContext *common.FilterContext, opts *list.Options) (
 	experiments []*model.Experiment, total_size int, nextPageToken string, err error) {
-	return r.experimentStore.ListExperiments(opts)
+	return r.experimentStore.ListExperiments(filterContext, opts)
 }
 
 func (r *ResourceManager) DeleteExperiment(experimentID string) error {
@@ -817,7 +821,7 @@ func (r *ResourceManager) CreateDefaultExperiment() (string, error) {
 	}
 
 	// Create default experiment
-	defaultExperiment := &model.Experiment{
+	defaultExperiment := &api.Experiment{
 		Name:        "Default",
 		Description: "All runs created without specifying an experiment will be grouped here.",
 	}
@@ -1017,6 +1021,14 @@ func (r *ResourceManager) IsRequestAuthorized(userIdentity string, namespace str
 	return r.kfamClient.IsAuthorized(userIdentity, namespace)
 }
 
+func (r *ResourceManager) GetNamespaceFromExperimentID(experimentID string) (string, error) {
+	experiment, err := r.GetExperiment(experimentID)
+	if err != nil {
+		return "", util.Wrap(err, "Failed to get namespace from experiment ID.")
+	}
+	return experiment.Namespace, nil
+}
+
 func (r *ResourceManager) GetNamespaceFromRunID(runId string) (string, error) {
 	runDetail, err := r.GetRun(runId)
 	if err != nil {
diff --git a/backend/src/apiserver/resource/resource_manager_test.go b/backend/src/apiserver/resource/resource_manager_test.go
index 3fcdeb0a4ec..3aa36865c48 100644
--- a/backend/src/apiserver/resource/resource_manager_test.go
+++ b/backend/src/apiserver/resource/resource_manager_test.go
@@ -88,8 +88,8 @@ func initWithExperiment(t *testing.T) (*FakeClientManager, *ResourceManager, *mo
 	initEnvVars()
 	store := NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch())
 	manager := NewResourceManager(store)
-	experiment := &model.Experiment{Name: "e1"}
-	experiment, err := manager.CreateExperiment(experiment)
+	apiExperiment := &api.Experiment{Name: "e1"}
+	experiment, err := manager.CreateExperiment(apiExperiment)
 	assert.Nil(t, err)
 	return store, manager, experiment
 }
@@ -98,8 +98,8 @@ func initWithExperimentAndPipeline(t *testing.T) (*FakeClientManager, *ResourceM
 	initEnvVars()
 	store := NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch())
 	manager := NewResourceManager(store)
-	experiment := &model.Experiment{Name: "e1"}
-	experiment, err := manager.CreateExperiment(experiment)
+	apiExperiment := &api.Experiment{Name: "e1"}
+	experiment, err := manager.CreateExperiment(apiExperiment)
 	assert.Nil(t, err)
 	pipeline, err := manager.CreatePipeline("p1", "", []byte(testWorkflow.ToStringForStore()))
 	assert.Nil(t, err)
@@ -307,8 +307,8 @@ func TestGetPipelineTemplate_PipelineFileNotFound(t *testing.T) {
 func TestCreateRun_ThroughPipelineID(t *testing.T) {
 	store, manager, p := initWithPipeline(t)
 	defer store.Close()
-	experiment := &model.Experiment{Name: "e1"}
-	experiment, err := manager.CreateExperiment(experiment)
+	apiExperiment := &api.Experiment{Name: "e1"}
+	experiment, err := manager.CreateExperiment(apiExperiment)
 	assert.Nil(t, err)
 	apiRun := &api.Run{
 		Name: "run1",
@@ -898,8 +898,8 @@ func TestCreateJob_ThroughWorkflowSpec(t *testing.T) {
 func TestCreateJob_ThroughPipelineID(t *testing.T) {
 	store, manager, pipeline := initWithPipeline(t)
 	defer store.Close()
-	experiment := &model.Experiment{Name: "e1"}
-	experiment, err := manager.CreateExperiment(experiment)
+	apiExperiment := &api.Experiment{Name: "e1"}
+	experiment, err := manager.CreateExperiment(apiExperiment)
 	job := &api.Job{
 		Name:    "j1",
 		Enabled: true,
diff --git a/backend/src/apiserver/server/api_converter.go b/backend/src/apiserver/server/api_converter.go
index 451013ea050..0725f42a3b3 100644
--- a/backend/src/apiserver/server/api_converter.go
+++ b/backend/src/apiserver/server/api_converter.go
@@ -26,6 +26,23 @@ import (
 )
 
 func ToApiExperiment(experiment *model.Experiment) *api.Experiment {
+	if common.IsMultiUserMode() {
+		return &api.Experiment{
+			Id:          experiment.UUID,
+			Name:        experiment.Name,
+			Description: experiment.Description,
+			CreatedAt:   &timestamp.Timestamp{Seconds: experiment.CreatedAtInSec},
+			ResourceReferences: []*api.ResourceReference{
+				&api.ResourceReference{
+					Key: &api.ResourceKey{
+						Type: api.ResourceType_NAMESPACE,
+						Id:   experiment.Namespace,
+					},
+					Relationship: api.Relationship_OWNER,
+				},
+			},
+		}
+	}
 	return &api.Experiment{
 		Id:          experiment.UUID,
 		Name:        experiment.Name,
@@ -42,13 +59,6 @@ func ToApiExperiments(experiments []*model.Experiment) []*api.Experiment {
 	return apiExperiments
 }
 
-func ToModelExperiment(experiment *api.Experiment) *model.Experiment {
-	return &model.Experiment{
-		Name:        experiment.Name,
-		Description: experiment.Description,
-	}
-}
-
 func ToApiPipeline(pipeline *model.Pipeline) *api.Pipeline {
 	params, err := toApiParameters(pipeline.Parameters)
 	if err != nil {
diff --git a/backend/src/apiserver/server/experiment_server.go b/backend/src/apiserver/server/experiment_server.go
index 61ac9bfd49a..ed5deaebd05 100644
--- a/backend/src/apiserver/server/experiment_server.go
+++ b/backend/src/apiserver/server/experiment_server.go
@@ -5,9 +5,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 ExperimentServer struct {
@@ -20,7 +22,13 @@ func (s *ExperimentServer) CreateExperiment(ctx context.Context, request *api.Cr
 	if err != nil {
 		return nil, util.Wrap(err, "Validate experiment request failed.")
 	}
-	newExperiment, err := s.resourceManager.CreateExperiment(ToModelExperiment(request.Experiment))
+
+	err = CanAccessNamespaceInResourceReferences(s.resourceManager, ctx, request.Experiment.ResourceReferences)
+	if err != nil {
+		return nil, util.Wrap(err, "Failed to authorize the requests.")
+	}
+
+	newExperiment, err := s.resourceManager.CreateExperiment(request.Experiment)
 	if err != nil {
 		return nil, util.Wrap(err, "Create experiment failed.")
 	}
@@ -29,6 +37,11 @@ func (s *ExperimentServer) CreateExperiment(ctx context.Context, request *api.Cr
 
 func (s *ExperimentServer) GetExperiment(ctx context.Context, request *api.GetExperimentRequest) (
 	*api.Experiment, error) {
+	err := s.canAccessExperiment(ctx, request.Id)
+	if err != nil {
+		return nil, util.Wrap(err, "Failed to authorize the requests.")
+	}
+
 	experiment, err := s.resourceManager.GetExperiment(request.Id)
 	if err != nil {
 		return nil, util.Wrap(err, "Get experiment failed.")
@@ -44,7 +57,17 @@ func (s *ExperimentServer) ListExperiment(ctx context.Context, request *api.List
 		return nil, util.Wrap(err, "Failed to create list options")
 	}
 
-	experiments, total_size, nextPageToken, err := s.resourceManager.ListExperiments(opts)
+	filterContext, err := ValidateFilter(request.ResourceReferenceKey)
+	if err != nil {
+		return nil, util.Wrap(err, "Validating filter failed.")
+	}
+
+	refKey := filterContext.ReferenceKey
+	if refKey != nil && refKey.Type != common.Namespace {
+		return nil, util.NewInvalidInputError("List experiment only supports filtering by namespace.")
+	}
+
+	experiments, total_size, nextPageToken, err := s.resourceManager.ListExperiments(filterContext, opts)
 	if err != nil {
 		return nil, util.Wrap(err, "List experiments failed.")
 	}
@@ -56,7 +79,12 @@ func (s *ExperimentServer) ListExperiment(ctx context.Context, request *api.List
 }
 
 func (s *ExperimentServer) DeleteExperiment(ctx context.Context, request *api.DeleteExperimentRequest) (*empty.Empty, error) {
-	err := s.resourceManager.DeleteExperiment(request.Id)
+	err := s.canAccessExperiment(ctx, request.Id)
+	if err != nil {
+		return nil, util.Wrap(err, "Failed to authorize the requests.")
+	}
+
+	err = s.resourceManager.DeleteExperiment(request.Id)
 	if err != nil {
 		return nil, err
 	}
@@ -67,6 +95,36 @@ func ValidateCreateExperimentRequest(request *api.CreateExperimentRequest) error
 	if request.Experiment == nil || request.Experiment.Name == "" {
 		return util.NewInvalidInputError("Experiment name is empty. Please specify a valid experiment name.")
 	}
+
+	if common.IsMultiUserMode() {
+		if len(request.Experiment.ResourceReferences) != 1 {
+			return util.NewInvalidInputError("Experiment should specify one and only one resource reference.")
+		}
+		namespace := common.GetNamespaceFromAPIResourceReferences(request.Experiment.ResourceReferences)
+		if len(namespace) == 0 {
+			return util.NewInvalidInputError("Experiment should specify namespace.")
+		}
+	}
+	return nil
+}
+
+func (s *ExperimentServer) canAccessExperiment(ctx context.Context, experimentID string) error {
+	if !common.IsMultiUserMode() {
+		// Skip authorization if not multi-user mode.
+		return nil
+	}
+	namespace, err := s.resourceManager.GetNamespaceFromExperimentID(experimentID)
+	if err != nil {
+		return util.Wrap(err, "Failed to authorize with the experiment ID.")
+	}
+	if len(namespace) == 0 {
+		return util.NewInternalServerError(errors.New("There is no namespace found"), "There is no namespace found")
+	}
+
+	err = isAuthorized(s.resourceManager, ctx, namespace)
+	if err != nil {
+		return util.Wrap(err, "Failed to authorize with API resource references")
+	}
 	return nil
 }
 
diff --git a/backend/src/apiserver/server/experiment_server_test.go b/backend/src/apiserver/server/experiment_server_test.go
index 28cb34221d8..c4e7a0a6575 100644
--- a/backend/src/apiserver/server/experiment_server_test.go
+++ b/backend/src/apiserver/server/experiment_server_test.go
@@ -1,13 +1,18 @@
 package server
 
 import (
+	"context"
+	"strings"
 	"testing"
 
 	"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/apiserver/resource"
 	"github.com/kubeflow/pipelines/backend/src/common/util"
+	"github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
+	"google.golang.org/grpc/metadata"
 )
 
 func TestCreateExperiment(t *testing.T) {
@@ -38,6 +43,39 @@ func TestCreateExperiment_Failed(t *testing.T) {
 	assert.Contains(t, err.Error(), "Create experiment failed.")
 }
 
+func TestCreateExperiment_Multiuser(t *testing.T) {
+	viper.Set(common.MultiUserMode, "true")
+	defer viper.Set(common.MultiUserMode, "false")
+	md := metadata.New(map[string]string{common.GoogleIAPUserIdentityHeader: "accounts.google.com:user@google.com"})
+	ctx := metadata.NewIncomingContext(context.Background(), md)
+
+	clientManager := resource.NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch())
+	resourceManager := resource.NewResourceManager(clientManager)
+	server := ExperimentServer{resourceManager: resourceManager}
+	resourceReferences := []*api.ResourceReference{
+		{
+			Key:          &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns1"},
+			Relationship: api.Relationship_OWNER,
+		},
+	}
+	experiment := &api.Experiment{
+		Name:               "exp1",
+		Description:        "first experiment",
+		ResourceReferences: resourceReferences,
+	}
+
+	result, err := server.CreateExperiment(ctx, &api.CreateExperimentRequest{Experiment: experiment})
+	assert.Nil(t, err)
+	expectedExperiment := &api.Experiment{
+		Id:                 resource.DefaultFakeUUID,
+		Name:               "exp1",
+		Description:        "first experiment",
+		CreatedAt:          &timestamp.Timestamp{Seconds: 1},
+		ResourceReferences: resourceReferences,
+	}
+	assert.Equal(t, expectedExperiment, result)
+}
+
 func TestGetExperiment(t *testing.T) {
 	clientManager := resource.NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch())
 	resourceManager := resource.NewResourceManager(clientManager)
@@ -70,6 +108,40 @@ func TestGetExperiment_Failed(t *testing.T) {
 	assert.Contains(t, err.Error(), "Get experiment failed.")
 }
 
+func TestGetExperiment_Multiuser(t *testing.T) {
+	viper.Set(common.MultiUserMode, "true")
+	defer viper.Set(common.MultiUserMode, "false")
+	md := metadata.New(map[string]string{common.GoogleIAPUserIdentityHeader: "accounts.google.com:user@google.com"})
+	ctx := metadata.NewIncomingContext(context.Background(), md)
+
+	clientManager := resource.NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch())
+	resourceManager := resource.NewResourceManager(clientManager)
+	server := ExperimentServer{resourceManager: resourceManager}
+	resourceReferences := []*api.ResourceReference{
+		{
+			Key:          &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns1"},
+			Relationship: api.Relationship_OWNER,
+		},
+	}
+	experiment := &api.Experiment{
+		Name:               "exp1",
+		Description:        "first experiment",
+		ResourceReferences: resourceReferences,
+	}
+
+	createResult, err := server.CreateExperiment(ctx, &api.CreateExperimentRequest{Experiment: experiment})
+	assert.Nil(t, err)
+	result, err := server.GetExperiment(ctx, &api.GetExperimentRequest{Id: createResult.Id})
+	expectedExperiment := &api.Experiment{
+		Id:                 createResult.Id,
+		Name:               "exp1",
+		Description:        "first experiment",
+		CreatedAt:          &timestamp.Timestamp{Seconds: 1},
+		ResourceReferences: resourceReferences,
+	}
+	assert.Equal(t, expectedExperiment, result)
+}
+
 func TestListExperiment(t *testing.T) {
 	clientManager := resource.NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch())
 	resourceManager := resource.NewResourceManager(clientManager)
@@ -102,9 +174,165 @@ func TestListExperiment_Failed(t *testing.T) {
 	assert.Contains(t, err.Error(), "List experiments failed.")
 }
 
-func TestValidateCreateExperimentRequest_EmptyName(t *testing.T) {
-	experiment := &api.Experiment{Description: "first experiment"}
-	err := ValidateCreateExperimentRequest(&api.CreateExperimentRequest{Experiment: experiment})
-	assert.NotNil(t, err)
-	assert.Contains(t, err.Error(), "name is empty")
+func TestListExperiment_Multiuser(t *testing.T) {
+	viper.Set(common.MultiUserMode, "true")
+	defer viper.Set(common.MultiUserMode, "false")
+	md := metadata.New(map[string]string{common.GoogleIAPUserIdentityHeader: "accounts.google.com:user@google.com"})
+	ctx := metadata.NewIncomingContext(context.Background(), md)
+
+	clientManager := resource.NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch())
+	resourceManager := resource.NewResourceManager(clientManager)
+	server := ExperimentServer{resourceManager: resourceManager}
+	referenceKey := &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns1"}
+	resourceReferences := []*api.ResourceReference{
+		{
+			Key:          referenceKey,
+			Relationship: api.Relationship_OWNER,
+		},
+	}
+	experiment := &api.Experiment{
+		Name:               "exp1",
+		Description:        "first experiment",
+		ResourceReferences: resourceReferences,
+	}
+
+	createResult, err := server.CreateExperiment(ctx, &api.CreateExperimentRequest{Experiment: experiment})
+	assert.Nil(t, err)
+
+	result, err := server.ListExperiment(ctx, &api.ListExperimentsRequest{ResourceReferenceKey: referenceKey})
+	assert.Nil(t, err)
+	expectedExperiment := []*api.Experiment{{
+		Id:                 createResult.Id,
+		Name:               "exp1",
+		Description:        "first experiment",
+		CreatedAt:          &timestamp.Timestamp{Seconds: 1},
+		ResourceReferences: resourceReferences,
+	}}
+	assert.Equal(t, expectedExperiment, result.Experiments)
+
+	result, err = server.ListExperiment(ctx, &api.ListExperimentsRequest{ResourceReferenceKey: &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns2"}})
+	assert.Nil(t, err)
+	expectedExperiment = []*api.Experiment{}
+	assert.Equal(t, expectedExperiment, result.Experiments)
+}
+
+func TestValidateCreateExperimentRequest(t *testing.T) {
+	tests := []struct {
+		name         string
+		experiment   *api.Experiment
+		wantError    bool
+		errorMessage string
+	}{
+		{
+			"Valid",
+			&api.Experiment{Name: "exp1", Description: "first experiment"},
+			false,
+			"",
+		},
+		{
+			"Empty name",
+			&api.Experiment{Description: "first experiment"},
+			true,
+			"name is empty",
+		},
+	}
+
+	for _, tc := range tests {
+		err := ValidateCreateExperimentRequest(&api.CreateExperimentRequest{Experiment: tc.experiment})
+		if !tc.wantError && err != nil {
+			t.Errorf("TestValidateCreateExperimentRequest(%v) expect no error but got %v", tc.name, err)
+		}
+		if tc.wantError {
+			if err == nil {
+				t.Errorf("TestValidateCreateExperimentRequest(%v) expect error but got nil", tc.name)
+			} else if !strings.Contains(err.Error(), tc.errorMessage) {
+				t.Errorf("TestValidateCreateExperimentRequest(%v) expect error containing: %v, but got: %v", tc.name, tc.errorMessage, err)
+			}
+		}
+	}
+}
+
+func TestValidateCreateExperimentRequest_Multiuser(t *testing.T) {
+	viper.Set(common.MultiUserMode, "true")
+	defer viper.Set(common.MultiUserMode, "false")
+	tests := []struct {
+		name         string
+		experiment   *api.Experiment
+		wantError    bool
+		errorMessage string
+	}{
+		{
+			"Valid",
+			&api.Experiment{
+				Name:        "exp1",
+				Description: "first experiment",
+				ResourceReferences: []*api.ResourceReference{
+					{
+						Key:          &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns1"},
+						Relationship: api.Relationship_OWNER,
+					},
+				},
+			},
+			false,
+			"",
+		},
+		{
+			"Missing namespace",
+			&api.Experiment{
+				Name:               "exp1",
+				Description:        "first experiment",
+				ResourceReferences: []*api.ResourceReference{},
+			},
+			true,
+			"Experiment should specify one and only one resource reference",
+		},
+		{
+			"Multiple namespace",
+			&api.Experiment{
+				Name:        "exp1",
+				Description: "first experiment",
+				ResourceReferences: []*api.ResourceReference{
+					{
+						Key:          &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns1"},
+						Relationship: api.Relationship_OWNER,
+					},
+					{
+						Key:          &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns2"},
+						Relationship: api.Relationship_OWNER,
+					},
+				},
+			},
+			true,
+			"Experiment should specify one and only one resource reference",
+		},
+		{
+			"Invalid resource type",
+			&api.Experiment{
+				Name:        "exp1",
+				Description: "first experiment",
+				ResourceReferences: []*api.ResourceReference{
+					{
+						Key:          &api.ResourceKey{Type: api.ResourceType_EXPERIMENT, Id: "exp2"},
+						Relationship: api.Relationship_OWNER,
+					},
+				},
+			},
+			true,
+			"Experiment should specify namespace",
+		},
+	}
+
+	for _, tc := range tests {
+		err := ValidateCreateExperimentRequest(&api.CreateExperimentRequest{Experiment: tc.experiment})
+		if !tc.wantError && err != nil {
+			t.Errorf("TestValidateCreateExperimentRequest(%v) expect no error but got %v", tc.name, err)
+		}
+		if tc.wantError {
+			if err == nil {
+				t.Errorf("TestValidateCreateExperimentRequest(%v) expect error but got nil", tc.name)
+			} else if !strings.Contains(err.Error(), tc.errorMessage) {
+				t.Errorf("TestValidateCreateExperimentRequest(%v) expect error containing: %v, but got: %v", tc.name, tc.errorMessage, err)
+			}
+		}
+	}
 }
diff --git a/backend/src/apiserver/server/run_server_test.go b/backend/src/apiserver/server/run_server_test.go
index a01644674b7..cff6d0def02 100644
--- a/backend/src/apiserver/server/run_server_test.go
+++ b/backend/src/apiserver/server/run_server_test.go
@@ -377,10 +377,13 @@ func TestReportRunMetrics_PartialFailures(t *testing.T) {
 }
 
 func TestCanAccessRun_Unauthorized(t *testing.T) {
+	viper.Set(common.MultiUserMode, "true")
+	defer viper.Set(common.MultiUserMode, "false")
+
 	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)
 
@@ -410,10 +413,13 @@ func TestCanAccessRun_Unauthorized(t *testing.T) {
 }
 
 func TestCanAccessRun_Authorized(t *testing.T) {
+	viper.Set(common.MultiUserMode, "true")
+	defer viper.Set(common.MultiUserMode, "false")
+
 	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)
 
diff --git a/backend/src/apiserver/server/test_util.go b/backend/src/apiserver/server/test_util.go
index 779de5830de..f3bd28f6e71 100644
--- a/backend/src/apiserver/server/test_util.go
+++ b/backend/src/apiserver/server/test_util.go
@@ -82,8 +82,19 @@ func initWithExperiment(t *testing.T) (*resource.FakeClientManager, *resource.Re
 	initEnvVars()
 	clientManager := resource.NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch())
 	resourceManager := resource.NewResourceManager(clientManager)
-	experiment := &model.Experiment{Name: "123"}
-	experiment, err := resourceManager.CreateExperiment(experiment)
+	apiExperiment := &api.Experiment{Name: "123"}
+	if common.IsMultiUserMode() {
+		apiExperiment = &api.Experiment{
+			Name: "123",
+			ResourceReferences: []*api.ResourceReference{
+				{
+					Key:          &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns1"},
+					Relationship: api.Relationship_OWNER,
+				},
+			},
+		}
+	}
+	experiment, err := resourceManager.CreateExperiment(apiExperiment)
 	assert.Nil(t, err)
 	return clientManager, resourceManager, experiment
 }
@@ -93,8 +104,19 @@ func initWithExperiment_KFAM_Unauthorized(t *testing.T) (*resource.FakeClientMan
 	clientManager := resource.NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch())
 	clientManager.KfamClientFake = client.NewFakeKFAMClientUnauthorized()
 	resourceManager := resource.NewResourceManager(clientManager)
-	experiment := &model.Experiment{Name: "123"}
-	experiment, err := resourceManager.CreateExperiment(experiment)
+	apiExperiment := &api.Experiment{Name: "123"}
+	if common.IsMultiUserMode() {
+		apiExperiment = &api.Experiment{
+			Name: "123",
+			ResourceReferences: []*api.ResourceReference{
+				{
+					Key:          &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns1"},
+					Relationship: api.Relationship_OWNER,
+				},
+			},
+		}
+	}
+	experiment, err := resourceManager.CreateExperiment(apiExperiment)
 	assert.Nil(t, err)
 	return clientManager, resourceManager, experiment
 }
@@ -105,8 +127,8 @@ func initWithExperimentAndPipelineVersion(t *testing.T) (*resource.FakeClientMan
 	resourceManager := resource.NewResourceManager(clientManager)
 
 	// Create an experiment.
-	experiment := &model.Experiment{Name: "123"}
-	experiment, err := resourceManager.CreateExperiment(experiment)
+	apiExperiment := &api.Experiment{Name: "123"}
+	experiment, err := resourceManager.CreateExperiment(apiExperiment)
 	assert.Nil(t, err)
 
 	// Create a pipeline and then a pipeline version.
diff --git a/backend/src/apiserver/server/util_test.go b/backend/src/apiserver/server/util_test.go
index cb9515d3a91..957eed14884 100644
--- a/backend/src/apiserver/server/util_test.go
+++ b/backend/src/apiserver/server/util_test.go
@@ -346,9 +346,12 @@ func TestGetUserIdentity(t *testing.T) {
 }
 
 func TestCanAccessNamespaceInResourceReferencesUnauthorized(t *testing.T) {
+	viper.Set(common.MultiUserMode, "true")
+	defer viper.Set(common.MultiUserMode, "false")
+
 	clients, manager, _ := 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)
 	references := []*api.ResourceReference{
@@ -363,9 +366,12 @@ func TestCanAccessNamespaceInResourceReferencesUnauthorized(t *testing.T) {
 }
 
 func TestCanAccessNamespaceInResourceReferences_Authorized(t *testing.T) {
+	viper.Set(common.MultiUserMode, "true")
+	defer viper.Set(common.MultiUserMode, "false")
+
 	clients, manager, _ := 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)
 	references := []*api.ResourceReference{
diff --git a/backend/src/apiserver/storage/experiment_store.go b/backend/src/apiserver/storage/experiment_store.go
index d62998f9c22..cd70ebe930e 100644
--- a/backend/src/apiserver/storage/experiment_store.go
+++ b/backend/src/apiserver/storage/experiment_store.go
@@ -14,7 +14,7 @@ import (
 )
 
 type ExperimentStoreInterface interface {
-	ListExperiments(opts *list.Options) ([]*model.Experiment, int, string, error)
+	ListExperiments(filterContext *common.FilterContext, opts *list.Options) ([]*model.Experiment, int, string, error)
 	GetExperiment(uuid string) (*model.Experiment, error)
 	CreateExperiment(*model.Experiment) (*model.Experiment, error)
 	DeleteExperiment(uuid string) error
@@ -30,13 +30,18 @@ type ExperimentStore struct {
 
 // Runs two SQL queries in a transaction to return a list of matching experiments, as well as their
 // total_size. The total_size does not reflect the page size.
-func (s *ExperimentStore) ListExperiments(opts *list.Options) ([]*model.Experiment, int, string, error) {
+func (s *ExperimentStore) ListExperiments(filterContext *common.FilterContext, opts *list.Options) ([]*model.Experiment, int, string, error) {
 	errorF := func(err error) ([]*model.Experiment, int, string, error) {
 		return nil, 0, "", util.NewInternalServerError(err, "Failed to list experiments: %v", err)
 	}
 
 	// SQL for getting the filtered and paginated rows
-	sqlBuilder := opts.AddFilterToSelect(sq.Select("*").From("experiments"))
+	sqlBuilder := sq.Select("*").From("experiments")
+	if filterContext.ReferenceKey != nil && filterContext.ReferenceKey.Type == common.Namespace {
+		sqlBuilder = sqlBuilder.Where(sq.Eq{"Namespace": filterContext.ReferenceKey.ID})
+	}
+	sqlBuilder = opts.AddFilterToSelect(sqlBuilder)
+
 	rowsSql, rowsArgs, err := opts.AddPaginationToSelect(sqlBuilder).ToSql()
 	if err != nil {
 		return errorF(err)
@@ -44,7 +49,11 @@ func (s *ExperimentStore) ListExperiments(opts *list.Options) ([]*model.Experime
 
 	// SQL for getting total size. This matches the query to get all the rows above, in order
 	// to do the same filter, but counts instead of scanning the rows.
-	sizeSql, sizeArgs, err := opts.AddFilterToSelect(sq.Select("count(*)").From("experiments")).ToSql()
+	sqlBuilder = sq.Select("count(*)").From("experiments")
+	if filterContext.ReferenceKey != nil && filterContext.ReferenceKey.Type == common.Namespace {
+		sqlBuilder = sqlBuilder.Where(sq.Eq{"Namespace": filterContext.ReferenceKey.ID})
+	}
+	sizeSql, sizeArgs, err := opts.AddFilterToSelect(sqlBuilder).ToSql()
 	if err != nil {
 		return errorF(err)
 	}
diff --git a/backend/src/apiserver/storage/experiment_store_test.go b/backend/src/apiserver/storage/experiment_store_test.go
index bbbac976a2e..8148a05bac3 100644
--- a/backend/src/apiserver/storage/experiment_store_test.go
+++ b/backend/src/apiserver/storage/experiment_store_test.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 
 	api "github.com/kubeflow/pipelines/backend/api/go_client"
+	"github.com/kubeflow/pipelines/backend/src/apiserver/common"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/list"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/model"
 	"github.com/kubeflow/pipelines/backend/src/common/util"
@@ -60,7 +61,7 @@ func TestListExperiments_Pagination(t *testing.T) {
 	opts, err := list.NewOptions(&model.Experiment{}, 2, "name", nil)
 	assert.Nil(t, err)
 
-	experiments, total_size, nextPageToken, err := experimentStore.ListExperiments(opts)
+	experiments, total_size, nextPageToken, err := experimentStore.ListExperiments(&common.FilterContext{}, opts)
 
 	assert.Nil(t, err)
 	assert.NotEmpty(t, nextPageToken)
@@ -84,7 +85,7 @@ func TestListExperiments_Pagination(t *testing.T) {
 	opts, err = list.NewOptionsFromToken(nextPageToken, 2)
 	assert.Nil(t, err)
 
-	experiments, total_size, nextPageToken, err = experimentStore.ListExperiments(opts)
+	experiments, total_size, nextPageToken, err = experimentStore.ListExperiments(&common.FilterContext{}, opts)
 	assert.Nil(t, err)
 	assert.Empty(t, nextPageToken)
 	assert.Equal(t, 4, total_size)
@@ -119,7 +120,7 @@ func TestListExperiments_Pagination_Descend(t *testing.T) {
 
 	opts, err := list.NewOptions(&model.Experiment{}, 2, "name desc", nil)
 	assert.Nil(t, err)
-	experiments, total_size, nextPageToken, err := experimentStore.ListExperiments(opts)
+	experiments, total_size, nextPageToken, err := experimentStore.ListExperiments(&common.FilterContext{}, opts)
 
 	assert.Nil(t, err)
 	assert.NotEmpty(t, nextPageToken)
@@ -143,7 +144,7 @@ func TestListExperiments_Pagination_Descend(t *testing.T) {
 	opts, err = list.NewOptionsFromToken(nextPageToken, 2)
 	assert.Nil(t, err)
 
-	experiments, total_size, nextPageToken, err = experimentStore.ListExperiments(opts)
+	experiments, total_size, nextPageToken, err = experimentStore.ListExperiments(&common.FilterContext{}, opts)
 	assert.Nil(t, err)
 	assert.Empty(t, nextPageToken)
 	assert.Equal(t, 4, total_size)
@@ -166,7 +167,7 @@ func TestListExperiments_Pagination_LessThanPageSize(t *testing.T) {
 	opts, err := list.NewOptions(&model.Experiment{}, 2, "", nil)
 	assert.Nil(t, err)
 
-	experiments, total_size, nextPageToken, err := experimentStore.ListExperiments(opts)
+	experiments, total_size, nextPageToken, err := experimentStore.ListExperiments(&common.FilterContext{}, opts)
 	assert.Nil(t, err)
 	assert.Equal(t, "", nextPageToken)
 	assert.Equal(t, 1, total_size)
@@ -181,7 +182,7 @@ func TestListExperimentsError(t *testing.T) {
 
 	opts, err := list.NewOptions(&model.Experiment{}, 2, "", nil)
 	assert.Nil(t, err)
-	_, _, _, err = experimentStore.ListExperiments(opts)
+	_, _, _, err = experimentStore.ListExperiments(&common.FilterContext{}, opts)
 	assert.Equal(t, codes.Internal, err.(*util.UserError).ExternalStatusCode())
 }
 
@@ -368,7 +369,7 @@ func TestListExperiments_Filtering(t *testing.T) {
 
 	opts, err := list.NewOptions(&model.Experiment{}, 2, "id", filterProto)
 	assert.Nil(t, err)
-	experiments, total_size, nextPageToken, err := experimentStore.ListExperiments(opts)
+	experiments, total_size, nextPageToken, err := experimentStore.ListExperiments(&common.FilterContext{}, opts)
 
 	expected := []*model.Experiment{
 		&model.Experiment{
@@ -394,7 +395,7 @@ func TestListExperiments_Filtering(t *testing.T) {
 	opts, err = list.NewOptionsFromToken(nextPageToken, 2)
 	assert.Nil(t, err)
 
-	experiments, total_size, nextPageToken, err = experimentStore.ListExperiments(opts)
+	experiments, total_size, nextPageToken, err = experimentStore.ListExperiments(&common.FilterContext{}, opts)
 
 	expected = []*model.Experiment{
 		&model.Experiment{

From b8a8c86628ab2a31f849ee7e2cb3f1bf0a6648ee Mon Sep 17 00:00:00 2001
From: Chen Sun <chensun@users.noreply.github.com>
Date: Sat, 14 Mar 2020 00:10:12 -0700
Subject: [PATCH 2/6] Address review comments and add additional unit tests

---
 .../src/apiserver/resource/model_converter.go |  7 +-
 .../resource/model_converter_test.go          | 89 +++++++++++++++++++
 .../resource/resource_manager_test.go         | 43 +++++++++
 .../src/apiserver/server/experiment_server.go | 16 ++--
 4 files changed, 146 insertions(+), 9 deletions(-)

diff --git a/backend/src/apiserver/resource/model_converter.go b/backend/src/apiserver/resource/model_converter.go
index fd3a3e615f5..90f5ffc43e0 100644
--- a/backend/src/apiserver/resource/model_converter.go
+++ b/backend/src/apiserver/resource/model_converter.go
@@ -22,16 +22,17 @@ import (
 	"github.com/kubeflow/pipelines/backend/src/apiserver/common"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/model"
 	"github.com/kubeflow/pipelines/backend/src/common/util"
+	"github.com/pkg/errors"
 )
 
 func (r *ResourceManager) ToModelExperiment(apiExperiment *api.Experiment) (*model.Experiment, error) {
 	namespace := ""
-	if common.IsMultiUserMode() {
-		resourceReferences := apiExperiment.GetResourceReferences()
+	resourceReferences := apiExperiment.GetResourceReferences()
+	if resourceReferences != nil {
 		if len(resourceReferences) != 1 ||
 			resourceReferences[0].Key.Type != api.ResourceType_NAMESPACE ||
 			resourceReferences[0].Relationship != api.Relationship_OWNER {
-			return nil, util.NewInvalidInputError("Invalid resource references for experiment: %v", resourceReferences)
+			return nil, util.NewInternalServerError(errors.New("Invalid resource references for experiment"), "Unable to convert to model experiment.")
 		}
 		namespace = resourceReferences[0].Key.Id
 	}
diff --git a/backend/src/apiserver/resource/model_converter_test.go b/backend/src/apiserver/resource/model_converter_test.go
index 12f3ef0fd77..2824a8dec90 100644
--- a/backend/src/apiserver/resource/model_converter_test.go
+++ b/backend/src/apiserver/resource/model_converter_test.go
@@ -15,10 +15,12 @@
 package resource
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
 	"github.com/golang/protobuf/ptypes/timestamp"
+	"github.com/google/go-cmp/cmp"
 	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"
@@ -33,6 +35,93 @@ func initResourceManager() (*FakeClientManager, *ResourceManager) {
 	return store, NewResourceManager(store)
 }
 
+func TestToModelExperiment(t *testing.T) {
+	store, manager := initResourceManager()
+	defer store.Close()
+
+	tests := []struct {
+		name                    string
+		experiment              *api.Experiment
+		wantError               bool
+		errorMessage            string
+		expectedModelExperiment *model.Experiment
+	}{
+		{
+			"No resource references",
+			&api.Experiment{
+				Name:        "exp1",
+				Description: "This is an experiment",
+			},
+			false,
+			"",
+			&model.Experiment{
+				Name:        "exp1",
+				Description: "This is an experiment",
+				Namespace:   "",
+			},
+		},
+		{
+			"Valid resource references",
+			&api.Experiment{
+				Name:        "exp1",
+				Description: "This is an experiment",
+				ResourceReferences: []*api.ResourceReference{
+					&api.ResourceReference{
+						Key: &api.ResourceKey{
+							Type: api.ResourceType_NAMESPACE,
+							Id:   "ns1",
+						},
+						Relationship: api.Relationship_OWNER,
+					},
+				},
+			},
+			false,
+			"",
+			&model.Experiment{
+				Name:        "exp1",
+				Description: "This is an experiment",
+				Namespace:   "ns1",
+			},
+		},
+		{
+			"Invalid resource references",
+			&api.Experiment{
+				Name:        "exp1",
+				Description: "This is an experiment",
+				ResourceReferences: []*api.ResourceReference{
+					&api.ResourceReference{
+						Key: &api.ResourceKey{
+							Type: api.ResourceType_EXPERIMENT,
+							Id:   "invalid",
+						},
+						Relationship: api.Relationship_OWNER,
+					},
+				},
+			},
+			true,
+			"Invalid resource references for experiment",
+			nil,
+		},
+	}
+
+	for _, tc := range tests {
+		modelExperiment, err := manager.ToModelExperiment(tc.experiment)
+		if tc.wantError {
+			if err == nil {
+				t.Errorf("TestToModelExperiment(%v) expect error but got nil", tc.name)
+			} else if !strings.Contains(err.Error(), tc.errorMessage) {
+				t.Errorf("TestToModelExperiment(%v) expect error containing: %v, but got: %v", tc.name, tc.errorMessage, err)
+			}
+		} else {
+			if err != nil {
+				t.Errorf("TestToModelExperiment(%v) expect no error but got %v", tc.name, err)
+			} else if !cmp.Equal(tc.expectedModelExperiment, modelExperiment) {
+				t.Errorf("TestToModelExperiment(%v) expect (%+v) but got (%+v)", tc.name, tc.expectedModelExperiment, modelExperiment)
+			}
+		}
+	}
+}
+
 func TestToModelRunMetric(t *testing.T) {
 	store, manager := initResourceManager()
 	defer store.Close()
diff --git a/backend/src/apiserver/resource/resource_manager_test.go b/backend/src/apiserver/resource/resource_manager_test.go
index 3aa36865c48..3b24684dc28 100644
--- a/backend/src/apiserver/resource/resource_manager_test.go
+++ b/backend/src/apiserver/resource/resource_manager_test.go
@@ -2290,3 +2290,46 @@ func TestDeletePipelineVersion_FileError(t *testing.T) {
 	assert.Nil(t, err)
 	assert.NotNil(t, version)
 }
+
+func TestCreateDefaultExperiment(t *testing.T) {
+	store := NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch())
+	defer store.Close()
+	manager := NewResourceManager(store)
+
+	experimentID, err := manager.CreateDefaultExperiment()
+	assert.Nil(t, err)
+	experiment, err := manager.GetExperiment(experimentID)
+	assert.Nil(t, err)
+
+	expectedExperiment := &model.Experiment{
+		UUID:           DefaultFakeUUID,
+		CreatedAtInSec: 1,
+		Name:           "Default",
+		Description:    "All runs created without specifying an experiment will be grouped here.",
+		Namespace:      "",
+	}
+	assert.Equal(t, expectedExperiment, experiment)
+}
+
+func TestCreateDefaultExperiment_MultiUser(t *testing.T) {
+	viper.Set(common.MultiUserMode, "true")
+	defer viper.Set(common.MultiUserMode, "false")
+
+	store := NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch())
+	defer store.Close()
+	manager := NewResourceManager(store)
+
+	experimentID, err := manager.CreateDefaultExperiment()
+	assert.Nil(t, err)
+	experiment, err := manager.GetExperiment(experimentID)
+	assert.Nil(t, err)
+
+	expectedExperiment := &model.Experiment{
+		UUID:           DefaultFakeUUID,
+		CreatedAtInSec: 1,
+		Name:           "Default",
+		Description:    "All runs created without specifying an experiment will be grouped here.",
+		Namespace:      "",
+	}
+	assert.Equal(t, expectedExperiment, experiment)
+}
diff --git a/backend/src/apiserver/server/experiment_server.go b/backend/src/apiserver/server/experiment_server.go
index ed5deaebd05..f2700e0845c 100644
--- a/backend/src/apiserver/server/experiment_server.go
+++ b/backend/src/apiserver/server/experiment_server.go
@@ -25,7 +25,7 @@ func (s *ExperimentServer) CreateExperiment(ctx context.Context, request *api.Cr
 
 	err = CanAccessNamespaceInResourceReferences(s.resourceManager, ctx, request.Experiment.ResourceReferences)
 	if err != nil {
-		return nil, util.Wrap(err, "Failed to authorize the requests.")
+		return nil, util.Wrap(err, "Failed to authorize the request.")
 	}
 
 	newExperiment, err := s.resourceManager.CreateExperiment(request.Experiment)
@@ -39,7 +39,7 @@ func (s *ExperimentServer) GetExperiment(ctx context.Context, request *api.GetEx
 	*api.Experiment, error) {
 	err := s.canAccessExperiment(ctx, request.Id)
 	if err != nil {
-		return nil, util.Wrap(err, "Failed to authorize the requests.")
+		return nil, util.Wrap(err, "Failed to authorize the request.")
 	}
 
 	experiment, err := s.resourceManager.GetExperiment(request.Id)
@@ -81,7 +81,7 @@ func (s *ExperimentServer) ListExperiment(ctx context.Context, request *api.List
 func (s *ExperimentServer) DeleteExperiment(ctx context.Context, request *api.DeleteExperimentRequest) (*empty.Empty, error) {
 	err := s.canAccessExperiment(ctx, request.Id)
 	if err != nil {
-		return nil, util.Wrap(err, "Failed to authorize the requests.")
+		return nil, util.Wrap(err, "Failed to authorize the request.")
 	}
 
 	err = s.resourceManager.DeleteExperiment(request.Id)
@@ -97,12 +97,16 @@ func ValidateCreateExperimentRequest(request *api.CreateExperimentRequest) error
 	}
 
 	if common.IsMultiUserMode() {
-		if len(request.Experiment.ResourceReferences) != 1 {
-			return util.NewInvalidInputError("Experiment should specify one and only one resource reference.")
+		resourceReferences := request.Experiment.GetResourceReferences()
+		if len(resourceReferences) != 1 ||
+			resourceReferences[0].Key.Type != api.ResourceType_NAMESPACE ||
+			resourceReferences[0].Relationship != api.Relationship_OWNER {
+			return util.NewInvalidInputError(
+				"Invalid resource references for experiment. Expect one namespace type with owner relationship. Got: %v", resourceReferences)
 		}
 		namespace := common.GetNamespaceFromAPIResourceReferences(request.Experiment.ResourceReferences)
 		if len(namespace) == 0 {
-			return util.NewInvalidInputError("Experiment should specify namespace.")
+			return util.NewInvalidInputError("Experiment namespace is empty. Please specify a valid namespace.")
 		}
 	}
 	return nil

From 0ff252fb746b3d1d8d1fc1b72641d315cb138b59 Mon Sep 17 00:00:00 2001
From: Chen Sun <chensun@users.noreply.github.com>
Date: Sat, 14 Mar 2020 01:07:47 -0700
Subject: [PATCH 3/6] Update build file

---
 backend/src/apiserver/resource/BUILD.bazel | 1 +
 1 file changed, 1 insertion(+)

diff --git a/backend/src/apiserver/resource/BUILD.bazel b/backend/src/apiserver/resource/BUILD.bazel
index 7bcce3a9a48..9274c460470 100644
--- a/backend/src/apiserver/resource/BUILD.bazel
+++ b/backend/src/apiserver/resource/BUILD.bazel
@@ -52,6 +52,7 @@ go_test(
         "//backend/src/crd/pkg/apis/scheduledworkflow/v1beta1:go_default_library",
         "@com_github_argoproj_argo//pkg/apis/workflow/v1alpha1:go_default_library",
         "@com_github_ghodss_yaml//:go_default_library",
+        "@com_github_google_go_cmp//cmp:go_default_library",
         "@com_github_pkg_errors//:go_default_library",
         "@com_github_spf13_viper//:go_default_library",
         "@com_github_stretchr_testify//assert:go_default_library",

From 0b4b38d83ac25f99d9e7454dd293bb277581d720 Mon Sep 17 00:00:00 2001
From: Chen Sun <chensun@users.noreply.github.com>
Date: Sat, 14 Mar 2020 01:40:49 -0700
Subject: [PATCH 4/6] fix unit tests

---
 backend/src/apiserver/server/experiment_server_test.go | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/backend/src/apiserver/server/experiment_server_test.go b/backend/src/apiserver/server/experiment_server_test.go
index c4e7a0a6575..2d5fda0ede6 100644
--- a/backend/src/apiserver/server/experiment_server_test.go
+++ b/backend/src/apiserver/server/experiment_server_test.go
@@ -284,7 +284,7 @@ func TestValidateCreateExperimentRequest_Multiuser(t *testing.T) {
 				ResourceReferences: []*api.ResourceReference{},
 			},
 			true,
-			"Experiment should specify one and only one resource reference",
+			"Invalid resource references for experiment.",
 		},
 		{
 			"Multiple namespace",
@@ -303,7 +303,7 @@ func TestValidateCreateExperimentRequest_Multiuser(t *testing.T) {
 				},
 			},
 			true,
-			"Experiment should specify one and only one resource reference",
+			"Invalid resource references for experiment.",
 		},
 		{
 			"Invalid resource type",
@@ -318,7 +318,7 @@ func TestValidateCreateExperimentRequest_Multiuser(t *testing.T) {
 				},
 			},
 			true,
-			"Experiment should specify namespace",
+			"Invalid resource references for experiment.",
 		},
 	}
 

From d06f4d6c823d66646f38e81f3ab92f182f47113c Mon Sep 17 00:00:00 2001
From: Chen Sun <chensun@users.noreply.github.com>
Date: Mon, 16 Mar 2020 14:18:22 -0700
Subject: [PATCH 5/6] Reject empty namespace in multi user mode

---
 .../src/apiserver/server/experiment_server.go |  20 ++-
 .../server/experiment_server_test.go          | 124 +++++++++++++++---
 2 files changed, 120 insertions(+), 24 deletions(-)

diff --git a/backend/src/apiserver/server/experiment_server.go b/backend/src/apiserver/server/experiment_server.go
index f2700e0845c..ea26ca03dcd 100644
--- a/backend/src/apiserver/server/experiment_server.go
+++ b/backend/src/apiserver/server/experiment_server.go
@@ -62,9 +62,19 @@ func (s *ExperimentServer) ListExperiment(ctx context.Context, request *api.List
 		return nil, util.Wrap(err, "Validating filter failed.")
 	}
 
-	refKey := filterContext.ReferenceKey
-	if refKey != nil && refKey.Type != common.Namespace {
-		return nil, util.NewInvalidInputError("List experiment only supports filtering by namespace.")
+	if common.IsMultiUserMode() {
+		refKey := filterContext.ReferenceKey
+		if refKey == nil || refKey.Type != common.Namespace {
+			return nil, util.NewInvalidInputError("Invalid resource references for experiment. ListExperiment requires filtering by namespace.")
+		}
+		if len(refKey.ID) == 0 {
+			return nil, util.NewInvalidInputError("Invalid resource references for experiment. Namespace is empty.")
+		}
+	} else {
+		// In single user mode, apply filter with empty namespace for backward compatibile.
+		filterContext = &common.FilterContext{
+			ReferenceKey: &common.ReferenceKey{Type: common.Namespace, ID: ""},
+		}
 	}
 
 	experiments, total_size, nextPageToken, err := s.resourceManager.ListExperiments(filterContext, opts)
@@ -106,7 +116,7 @@ func ValidateCreateExperimentRequest(request *api.CreateExperimentRequest) error
 		}
 		namespace := common.GetNamespaceFromAPIResourceReferences(request.Experiment.ResourceReferences)
 		if len(namespace) == 0 {
-			return util.NewInvalidInputError("Experiment namespace is empty. Please specify a valid namespace.")
+			return util.NewInvalidInputError("Invalid resource references for experiment. Namespace is empty.")
 		}
 	}
 	return nil
@@ -122,7 +132,7 @@ func (s *ExperimentServer) canAccessExperiment(ctx context.Context, experimentID
 		return util.Wrap(err, "Failed to authorize with the experiment ID.")
 	}
 	if len(namespace) == 0 {
-		return util.NewInternalServerError(errors.New("There is no namespace found"), "There is no namespace found")
+		return util.NewInternalServerError(errors.New("Empty namespace"), "The experiment doesn't have a valid namespace.")
 	}
 
 	err = isAuthorized(s.resourceManager, ctx, namespace)
diff --git a/backend/src/apiserver/server/experiment_server_test.go b/backend/src/apiserver/server/experiment_server_test.go
index 2d5fda0ede6..e9b6e33d8b9 100644
--- a/backend/src/apiserver/server/experiment_server_test.go
+++ b/backend/src/apiserver/server/experiment_server_test.go
@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/golang/protobuf/ptypes/timestamp"
+	"github.com/google/go-cmp/cmp"
 	api "github.com/kubeflow/pipelines/backend/api/go_client"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/common"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/resource"
@@ -177,16 +178,16 @@ func TestListExperiment_Failed(t *testing.T) {
 func TestListExperiment_Multiuser(t *testing.T) {
 	viper.Set(common.MultiUserMode, "true")
 	defer viper.Set(common.MultiUserMode, "false")
+
 	md := metadata.New(map[string]string{common.GoogleIAPUserIdentityHeader: "accounts.google.com:user@google.com"})
 	ctx := metadata.NewIncomingContext(context.Background(), md)
 
 	clientManager := resource.NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch())
 	resourceManager := resource.NewResourceManager(clientManager)
 	server := ExperimentServer{resourceManager: resourceManager}
-	referenceKey := &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns1"}
 	resourceReferences := []*api.ResourceReference{
 		{
-			Key:          referenceKey,
+			Key:          &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns1"},
 			Relationship: api.Relationship_OWNER,
 		},
 	}
@@ -199,21 +200,92 @@ func TestListExperiment_Multiuser(t *testing.T) {
 	createResult, err := server.CreateExperiment(ctx, &api.CreateExperimentRequest{Experiment: experiment})
 	assert.Nil(t, err)
 
-	result, err := server.ListExperiment(ctx, &api.ListExperimentsRequest{ResourceReferenceKey: referenceKey})
-	assert.Nil(t, err)
-	expectedExperiment := []*api.Experiment{{
-		Id:                 createResult.Id,
-		Name:               "exp1",
-		Description:        "first experiment",
-		CreatedAt:          &timestamp.Timestamp{Seconds: 1},
-		ResourceReferences: resourceReferences,
-	}}
-	assert.Equal(t, expectedExperiment, result.Experiments)
+	tests := []struct {
+		name                string
+		request             *api.ListExperimentsRequest
+		wantError           bool
+		errorMessage        string
+		expectedExperiments []*api.Experiment
+	}{
+		{
+			"Valid",
+			&api.ListExperimentsRequest{
+				ResourceReferenceKey: &api.ResourceKey{
+					Type: api.ResourceType_NAMESPACE,
+					Id:   "ns1",
+				},
+			},
+			false,
+			"",
+			[]*api.Experiment{{
+				Id:                 createResult.Id,
+				Name:               "exp1",
+				Description:        "first experiment",
+				CreatedAt:          &timestamp.Timestamp{Seconds: 1},
+				ResourceReferences: resourceReferences,
+			}},
+		},
+		{
+			"Valid but empty result",
+			&api.ListExperimentsRequest{
+				ResourceReferenceKey: &api.ResourceKey{
+					Type: api.ResourceType_NAMESPACE,
+					Id:   "ns2",
+				},
+			},
+			false,
+			"",
+			[]*api.Experiment{},
+		},
+		{
+			"Missing resource reference key",
+			&api.ListExperimentsRequest{},
+			true,
+			"Invalid resource references for experiment.",
+			nil,
+		},
+		{
+			"Invalid resource reference key type",
+			&api.ListExperimentsRequest{
+				ResourceReferenceKey: &api.ResourceKey{
+					Type: api.ResourceType_EXPERIMENT,
+					Id:   "fake_id",
+				},
+			},
+			true,
+			"Invalid resource references for experiment.",
+			nil,
+		},
+		{
+			"Empty namespace",
+			&api.ListExperimentsRequest{
+				ResourceReferenceKey: &api.ResourceKey{
+					Type: api.ResourceType_NAMESPACE,
+					Id:   "",
+				},
+			},
+			true,
+			"Invalid resource references for experiment. Namespace is empty.",
+			nil,
+		},
+	}
 
-	result, err = server.ListExperiment(ctx, &api.ListExperimentsRequest{ResourceReferenceKey: &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns2"}})
-	assert.Nil(t, err)
-	expectedExperiment = []*api.Experiment{}
-	assert.Equal(t, expectedExperiment, result.Experiments)
+	for _, tc := range tests {
+		response, err := server.ListExperiment(ctx, tc.request)
+		if tc.wantError {
+			if err == nil {
+				t.Errorf("TestListExperiment_Multiuser(%v) expect error but got nil", tc.name)
+			} else if !strings.Contains(err.Error(), tc.errorMessage) {
+				t.Errorf("TestListExperiment_Multiusert(%v) expect error containing: %v, but got: %v", tc.name, tc.errorMessage, err)
+			}
+		} else {
+			if err != nil {
+				t.Errorf("TestListExperiment_Multiuser(%v) expect no error but got %v", tc.name, err)
+			} else if !cmp.Equal(tc.expectedExperiments, response.Experiments) {
+				t.Errorf("TestListExperiment_Multiuser(%v) expect (%+v) but got (%+v)", tc.name, tc.expectedExperiments, response.Experiments)
+			}
+		}
+	}
 }
 
 func TestValidateCreateExperimentRequest(t *testing.T) {
@@ -279,13 +351,27 @@ func TestValidateCreateExperimentRequest_Multiuser(t *testing.T) {
 		{
 			"Missing namespace",
 			&api.Experiment{
-				Name:               "exp1",
-				Description:        "first experiment",
-				ResourceReferences: []*api.ResourceReference{},
+				Name:        "exp1",
+				Description: "first experiment",
 			},
 			true,
 			"Invalid resource references for experiment.",
 		},
+		{
+			"Empty namespace",
+			&api.Experiment{
+				Name:        "exp1",
+				Description: "first experiment",
+				ResourceReferences: []*api.ResourceReference{
+					{
+						Key:          &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: ""},
+						Relationship: api.Relationship_OWNER,
+					},
+				},
+			},
+			true,
+			"Invalid resource references for experiment. Namespace is empty.",
+		},
 		{
 			"Multiple namespace",
 			&api.Experiment{

From 0dcd2a139af0f24e612188f5e461221e6c2c1d13 Mon Sep 17 00:00:00 2001
From: Chen Sun <chensun@users.noreply.github.com>
Date: Tue, 17 Mar 2020 01:38:38 -0700
Subject: [PATCH 6/6] Address review comments

---
 backend/src/apiserver/server/api_converter.go | 28 ++++++++-----------
 .../src/apiserver/server/experiment_server.go |  1 +
 2 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/backend/src/apiserver/server/api_converter.go b/backend/src/apiserver/server/api_converter.go
index 0725f42a3b3..c10288c2d35 100644
--- a/backend/src/apiserver/server/api_converter.go
+++ b/backend/src/apiserver/server/api_converter.go
@@ -26,28 +26,24 @@ import (
 )
 
 func ToApiExperiment(experiment *model.Experiment) *api.Experiment {
+	resourceReferences := []*api.ResourceReference(nil)
 	if common.IsMultiUserMode() {
-		return &api.Experiment{
-			Id:          experiment.UUID,
-			Name:        experiment.Name,
-			Description: experiment.Description,
-			CreatedAt:   &timestamp.Timestamp{Seconds: experiment.CreatedAtInSec},
-			ResourceReferences: []*api.ResourceReference{
-				&api.ResourceReference{
-					Key: &api.ResourceKey{
-						Type: api.ResourceType_NAMESPACE,
-						Id:   experiment.Namespace,
-					},
-					Relationship: api.Relationship_OWNER,
+		resourceReferences = []*api.ResourceReference{
+			&api.ResourceReference{
+				Key: &api.ResourceKey{
+					Type: api.ResourceType_NAMESPACE,
+					Id:   experiment.Namespace,
 				},
+				Relationship: api.Relationship_OWNER,
 			},
 		}
 	}
 	return &api.Experiment{
-		Id:          experiment.UUID,
-		Name:        experiment.Name,
-		Description: experiment.Description,
-		CreatedAt:   &timestamp.Timestamp{Seconds: experiment.CreatedAtInSec},
+		Id:                 experiment.UUID,
+		Name:               experiment.Name,
+		Description:        experiment.Description,
+		CreatedAt:          &timestamp.Timestamp{Seconds: experiment.CreatedAtInSec},
+		ResourceReferences: resourceReferences,
 	}
 }
 
diff --git a/backend/src/apiserver/server/experiment_server.go b/backend/src/apiserver/server/experiment_server.go
index ea26ca03dcd..4ff394d804c 100644
--- a/backend/src/apiserver/server/experiment_server.go
+++ b/backend/src/apiserver/server/experiment_server.go
@@ -122,6 +122,7 @@ func ValidateCreateExperimentRequest(request *api.CreateExperimentRequest) error
 	return nil
 }
 
+// TODO(chensun): consider refactoring the code to get rid of double-query of experiment.
 func (s *ExperimentServer) canAccessExperiment(ctx context.Context, experimentID string) error {
 	if !common.IsMultiUserMode() {
 		// Skip authorization if not multi-user mode.