fix: return a codes.NotFound error when trying to get a non-existen…

…t repository (argoproj#7548) * fix: return a codes.NotFound error when trying to get a non-existent repository Signed-off-by: Simon Ninon <[email protected]> * move s.db.RepositoryExists call after the permission check Signed-off-by: Simon Ninon <[email protected]> * update ArgoDB mock and add unit tests Signed-off-by: Simon Ninon <[email protected]>
tkasuz · Oct 30, 2021 · f4fd836 · f4fd836
1 parent 2770c69
commit f4fd836
Show file tree

Hide file tree

Showing 5 changed files with 72 additions and 4 deletions.
diff --git a/server/repository/repository.go b/server/repository/repository.go
@@ -120,6 +120,15 @@ func (s *Server) Get(ctx context.Context, q *repositorypkg.RepoQuery) (*appsv1.R
 		return nil, err
 	}
 
+	// getRepo does not return an error for unconfigured repositories, so we are checking here
+	exists, err := s.db.RepositoryExists(ctx, q.Repo)
+	if err != nil {
+		return nil, err
+	}
+	if !exists {
+		return nil, status.Errorf(codes.NotFound, "repo '%s' not found", q.Repo)
+	}
+
 	// For backwards compatibility, if we have no repo type set assume a default
 	rType := repo.Type
 	if rType == "" {

diff --git a/server/repository/repository_test.go b/server/repository/repository_test.go
@@ -102,30 +102,53 @@ func TestRepositoryServer(t *testing.T) {
 		repoServerClient.On("TestRepository", mock.Anything, mock.Anything).Return(&apiclient.TestRepositoryResponse{}, nil)
 		repoServerClientset := mocks.Clientset{RepoServerServiceClient: &repoServerClient}
 
-		s := NewServer(&repoServerClientset, argoDB, enforcer, newFixtures().Cache, settingsMgr)
 		url := "https://test"
+		db := &dbmocks.ArgoDB{}
+		db.On("GetRepository", context.TODO(), url).Return(&v1alpha1.Repository{Repo: url}, nil)
+		db.On("RepositoryExists", context.TODO(), url).Return(true, nil)
+
+		s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, settingsMgr)
 		repo, err := s.Get(context.TODO(), &repository.RepoQuery{
 			Repo: url,
 		})
 		assert.Nil(t, err)
 		assert.Equal(t, repo.Repo, url)
 	})
 
-	t.Run("Test_GetWithNotExistRepoShouldReturn403", func(t *testing.T) {
+	t.Run("Test_GetWithErrorShouldReturn403", func(t *testing.T) {
 		repoServerClient := mocks.RepoServerServiceClient{}
 		repoServerClientset := mocks.Clientset{RepoServerServiceClient: &repoServerClient}
 
+		url := "https://test"
 		db := &dbmocks.ArgoDB{}
-		db.On("GetRepository", context.TODO(), "test").Return(nil, errors.New("not found"))
+		db.On("GetRepository", context.TODO(), url).Return(nil, errors.New("some error"))
+		db.On("RepositoryExists", context.TODO(), url).Return(true, nil)
 
 		s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, settingsMgr)
 		repo, err := s.Get(context.TODO(), &repository.RepoQuery{
-			Repo: "test",
+			Repo: url,
 		})
 		assert.Nil(t, repo)
 		assert.Equal(t, err.Error(), "rpc error: code = PermissionDenied desc = permission denied")
 	})
 
+	t.Run("Test_GetWithNotExistRepoShouldReturn404", func(t *testing.T) {
+		repoServerClient := mocks.RepoServerServiceClient{}
+		repoServerClientset := mocks.Clientset{RepoServerServiceClient: &repoServerClient}
+
+		url := "https://test"
+		db := &dbmocks.ArgoDB{}
+		db.On("GetRepository", context.TODO(), url).Return(&v1alpha1.Repository{Repo: url}, nil)
+		db.On("RepositoryExists", context.TODO(), url).Return(false, nil)
+
+		s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, settingsMgr)
+		repo, err := s.Get(context.TODO(), &repository.RepoQuery{
+			Repo: url,
+		})
+		assert.Nil(t, repo)
+		assert.Equal(t, err.Error(), "rpc error: code = NotFound desc = repo 'https://test' not found")
+	})
+
 	t.Run("Test_CreateRepositoryWithoutUpsert", func(t *testing.T) {
 		repoServerClient := mocks.RepoServerServiceClient{}
 		repoServerClient.On("TestRepository", mock.Anything, mock.Anything).Return(&apiclient.TestRepositoryResponse{}, nil)

diff --git a/util/db/db.go b/util/db/db.go
@@ -41,6 +41,8 @@ type ArgoDB interface {
 	CreateRepository(ctx context.Context, r *appv1.Repository) (*appv1.Repository, error)
 	// GetRepository returns a repository by URL
 	GetRepository(ctx context.Context, url string) (*appv1.Repository, error)
+	// RepositoryExists returns whether a repository is configured for the given URL
+	RepositoryExists(ctx context.Context, repoURL string) (bool, error)
 	// UpdateRepository updates a repository
 	UpdateRepository(ctx context.Context, r *appv1.Repository) (*appv1.Repository, error)
 	// DeleteRepository deletes a repository from config

diff --git a/util/db/mocks/ArgoDB.go b/util/db/mocks/ArgoDB.go
diff --git a/util/db/repository.go b/util/db/repository.go
@@ -84,6 +84,17 @@ func (db *db) GetRepository(ctx context.Context, repoURL string) (*appsv1.Reposi
 	return repository, err
 }
 
+func (db *db) RepositoryExists(ctx context.Context, repoURL string) (bool, error) {
+	secretsBackend := db.repoBackend()
+	exists, err := secretsBackend.RepositoryExists(ctx, repoURL)
+	if exists || err != nil {
+		return exists, err
+	}
+
+	legacyBackend := db.legacyRepoBackend()
+	return legacyBackend.RepositoryExists(ctx, repoURL)
+}
+
 func (db *db) getRepository(ctx context.Context, repoURL string) (*appsv1.Repository, error) {
 	secretsBackend := db.repoBackend()
 	exists, err := secretsBackend.RepositoryExists(ctx, repoURL)