Repository: argoproj/argo (go)

Issue:
Workflow Archive RBAC doesn't agree with Workflow / Timeline RBAC
Checklist:

 * [x] I've included the version.
 * [ ] I've included reproduction steps.
 * [ ] I've included the workflow YAML.
 * [x] I've included the logs.
 
**What happened**:
For context, I'm running the Argo Server in hosted mode, with the "client" auth mode enabled. The SA running the server has minimal RBAC (get/watch/list on configmaps and get on secrets).

Workflows are being written to the `argo_archived_workflows` table by my controller, but the Archived Workflows page in the UI claims there are none in the DB. Looking in the logs I found:
```
time="2020-03-09T13:32:52Z" level=debug msg=CanI allowed=false name=clinvar-ingest-pwnpk namespace=clinvar resource=workflows verb=get
```
Emphasis on `allowed=false`. This was confusing because the Timeline page makes the same call to decide what should be shown, and workflows have been appearing there without problems.

**What you expected to happen**:
All workflows in the archive table appear in the UI when using client-mode auth as a user with sufficient permissions.

**Anything else we need to know?**:
I found that the server is filtering archived workflows returned by the DB using a `can-i` call [here](https://github.com/argoproj/argo/blob/master/server/workflowarchive/archived_workflow_server.go#L73). When I run the command locally, it works:
```
$ kubectl -n clinvar auth can-i get workflow/clinvar-ingest-pwnpk
yes
```
Could the workflow archive API be dropping / ignoring the auth-mode config, always using the equivalent of server mode?

**Environment**:
- Argo version:
```
$ argo version
argo: v2.6.0
  BuildDate: 2020-02-28T23:32:45Z
  GitCommit: 5d3bdd56607eea962183a9e45009e3d08fafdf9b
  GitTreeState: clean
  GitTag: v2.6.0
  GoVersion: go1.13.4
  Compiler: gc
  Platform: darwin/amd64
```
I'm running 2.6.1 for the server / controller.
- Kubernetes version :
```
$ kubectl version -o yaml
clientVersion:
  buildDate: "2020-01-18T23:33:14Z"
  compiler: gc
  gitCommit: 2e808b7cb054ee242b68e62455323aa783991f03
  gitTreeState: clean
  gitVersion: v1.15.9
  goVersion: go1.12.12
  major: "1"
  minor: "15"
  platform: darwin/amd64
serverVersion:
  buildDate: "2020-01-29T19:27:22Z"
  compiler: gc
  gitCommit: 2ccf2c797713077f4a3d0f87c4b4bae725362326
  gitTreeState: clean
  gitVersion: v1.15.8-gke.3
  goVersion: go1.12.12b4
  major: "1"
  minor: 15+
  platform: linux/amd64
```

---
<!-- Issue Author: Don't delete this message to ecourage other users to support your issue! -->
**Message from the maintainers**:

If you are impacted by this bug please add a 👍 reaction to this issue! We often sort issues this way to know what to prioritize.


Pull request:
fix(archive): Fix bug that prevents listing archive workflows. Fixes …
Closes #2399

Checklist:

* [x] Either (a) I've created an [enhancement proposal](https://github.com/argoproj/argo/issues/new/choose) and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
* [x] The title of the PR is (a) [conventional](https://www.conventionalcommits.org/en/v1.0.0/), (b) states what changed, and (c) suffixes the related issues number. E.g. `"fix(controller): Updates such and such. Fixes #1234"`.  
* [x] I have written unit and/or e2e tests for my change. PRs without these are unlike to be merged.
* [x] Optional. I've added My organization is added to the USERS.md.
* [x] I've signed the CLA and required builds are green. 


Patch:
diff --git a/server/auth/authorizer.go b/server/auth/authorizer.go
index 51d167eda..ef9442a9d 100644
--- a/server/auth/authorizer.go
+++ b/server/auth/authorizer.go
@@ -5,8 +5,6 @@ import (
 
 	log "github.com/sirupsen/logrus"
 	authorizationv1 "k8s.io/api/authorization/v1"
-
-	"github.com/argoproj/argo/pkg/apis/workflow"
 )
 
 func CanI(ctx context.Context, verb, resource, namespace, name string) (bool, error) {
@@ -30,49 +28,3 @@ func CanI(ctx context.Context, verb, resource, namespace, name string) (bool, er
 	logCtx.WithField("status", review.Status).Debug("CanI")
 	return review.Status.Allowed, nil
 }
-
-type Authorizer struct {
-	ctx    context.Context
-	status map[string]authorizationv1.SubjectRulesReviewStatus
-}
-
-func (a Authorizer) CanI(verb, resource, namespace, name string) (bool, error) {
-	logCtx := log.WithFields(log.Fields{"verb": verb, "resource": resource, "namespace": namespace, "name": name})
-	_, ok := a.status[namespace]
-	if !ok {
-		kubeClientset := GetKubeClient(a.ctx)
-		review, err := kubeClientset.AuthorizationV1().SelfSubjectRulesReviews().Create(&authorizationv1.SelfSubjectRulesReview{Spec: authorizationv1.SelfSubjectRulesReviewSpec{Namespace: namespace}})
-		if err != nil {
-			return false, err
-		}
-		a.status[namespace] = review.Status
-	}
-	for _, rule := range a.status[namespace].ResourceRules {
-		if allowed(rule.Verbs, verb) &&
-			allowed(rule.Resources, resource) &&
-			allowed(rule.APIGroups, workflow.Group) &&
-			allowed(rule.ResourceNames, name) {
-			logCtx.WithFields(log.Fields{"rule": rule, "allowed": true}).Debug("CanI")
-			return true, nil
-		}
-	}
-	logCtx.WithField("allowed", false).Debug("CanI")
-	return false, nil
-}
-
-func NewAuthorizer(ctx context.Context) *Authorizer {
-	return &Authorizer{ctx, map[string]authorizationv1.SubjectRulesReviewStatus{}}
-}
-
-func allowed(values []string, value string) bool {
-	return len(values) == 0 || contains(values, "*") || contains(values, value)
-}
-
-func contains(values []string, value string) bool {
-	for _, s := range values {
-		if value == s {
-			return true
-		}
-	}
-	return false
-}
diff --git a/server/workflowarchive/archived_workflow_server.go b/server/workflowarchive/archived_workflow_server.go
index 1f87846ea..ea513ac49 100644
--- a/server/workflowarchive/archived_workflow_server.go
+++ b/server/workflowarchive/archived_workflow_server.go
@@ -52,22 +52,19 @@ func (w *archivedWorkflowServer) ListArchivedWorkflows(ctx context.Context, req
 	namespace := ""
 	minStartedAt := time.Time{}
 	maxStartedAt := time.Time{}
-	selectors := strings.Split(options.FieldSelector, ",")
-	for _, selector := range selectors {
+	for _, selector := range strings.Split(options.FieldSelector, ",") {
 		if len(selector) == 0 {
 			continue
 		}
 		if strings.HasPrefix(selector, "metadata.namespace=") {
 			namespace = strings.TrimPrefix(selector, "metadata.namespace=")
 		} else if strings.HasPrefix(selector, "spec.startedAt>") {
-			minStartedAtStr := strings.TrimPrefix(selector, "spec.startedAt>")
-			minStartedAt, err = time.Parse(time.RFC3339, minStartedAtStr)
+			minStartedAt, err = time.Parse(time.RFC3339, strings.TrimPrefix(selector, "spec.startedAt>"))
 			if err != nil {
 				return nil, err
 			}
 		} else if strings.HasPrefix(selector, "spec.startedAt<") {
-			maxStartedAtStr := strings.TrimPrefix(selector, "spec.startedAt<")
-			maxStartedAt, err = time.Parse(time.RFC3339, maxStartedAtStr)
+			maxStartedAt, err = time.Parse(time.RFC3339, strings.TrimPrefix(selector, "spec.startedAt<"))
 			if err != nil {
 				return nil, err
 			}
@@ -75,14 +72,19 @@ func (w *archivedWorkflowServer) ListArchivedWorkflows(ctx context.Context, req
 			return nil, fmt.Errorf("unsupported requirement %s", selector)
 		}
 	}
-
 	requirements, err := labels.ParseToRequirements(options.LabelSelector)
 	if err != nil {
 		return nil, err
 	}
 
 	items := make(wfv1.Workflows, 0)
-	authorizer := auth.NewAuthorizer(ctx)
+	allowed, err := auth.CanI(ctx, "list", workflow.WorkflowPlural, namespace, "")
+	if err != nil {
+		return nil, err
+	}
+	if !allowed {
+		return nil, status.Error(codes.PermissionDenied, "permission denied")
+	}
 	hasMore := true
 	// keep trying until we have enough
 	for len(items) < limit {
@@ -94,14 +96,7 @@ func (w *archivedWorkflowServer) ListArchivedWorkflows(ctx context.Context, req
 			if index == limit {
 				break
 			}
-			// TODO second pass filtering?
-			allowed, err := authorizer.CanI("get", workflow.WorkflowPlural, wf.Namespace, wf.Name)
-			if err != nil {
-				return nil, err
-			}
-			if allowed {
-				items = append(items, wf)
-			}
+			items = append(items, wf)
 		}
 		if len(moreItems) < limit+1 {
 			hasMore = false


Test patch:
diff --git a/server/auth/authorizer_test.go b/server/auth/authorizer_test.go
index a1ea59cc2..1d67fd635 100644
--- a/server/auth/authorizer_test.go
+++ b/server/auth/authorizer_test.go
@@ -36,11 +36,4 @@ func TestAuthorizer_CanI(t *testing.T) {
 			},
 		}, nil
 	})
-	t.Run("Authorizer.CanI", func(t *testing.T) {
-		authorizer := NewAuthorizer(ctx)
-		allowed, err := authorizer.CanI("", "", "", "my-name")
-		if assert.NoError(t, err) {
-			assert.True(t, allowed)
-		}
-	})
 }
diff --git a/server/workflowarchive/archived_workflow_server_test.go b/server/workflowarchive/archived_workflow_server_test.go
index 9079cb7bd..dbdd22578 100644
--- a/server/workflowarchive/archived_workflow_server_test.go
+++ b/server/workflowarchive/archived_workflow_server_test.go
@@ -71,12 +71,10 @@ func Test_archivedWorkflowServer(t *testing.T) {
 	ctx := context.WithValue(context.WithValue(context.TODO(), auth.WfKey, wfClient), auth.KubeKey, kubeClient)
 	t.Run("ListArchivedWorkflows", func(t *testing.T) {
 		allowed = false
-		resp, err := w.ListArchivedWorkflows(ctx, &workflowarchivepkg.ListArchivedWorkflowsRequest{ListOptions: &metav1.ListOptions{Limit: 1}})
-		if assert.NoError(t, err) {
-			assert.Len(t, resp.Items, 0)
-		}
+		_, err := w.ListArchivedWorkflows(ctx, &workflowarchivepkg.ListArchivedWorkflowsRequest{ListOptions: &metav1.ListOptions{Limit: 1}})
+		assert.Equal(t, err, status.Error(codes.PermissionDenied, "permission denied"))
 		allowed = true
-		resp, err = w.ListArchivedWorkflows(ctx, &workflowarchivepkg.ListArchivedWorkflowsRequest{ListOptions: &metav1.ListOptions{Limit: 1}})
+		resp, err := w.ListArchivedWorkflows(ctx, &workflowarchivepkg.ListArchivedWorkflowsRequest{ListOptions: &metav1.ListOptions{Limit: 1}})
 		if assert.NoError(t, err) {
 			assert.Len(t, resp.Items, 1)
 			assert.Equal(t, "1", resp.Continue)
diff --git a/test/e2e/argo_server_test.go b/test/e2e/argo_server_test.go
index 8cb729275..71fab24cd 100644
--- a/test/e2e/argo_server_test.go
+++ b/test/e2e/argo_server_test.go
@@ -317,10 +317,7 @@ func (s *ArgoServerSuite) TestPermission() {
 					WithQuery("listOptions.labelSelector", "argo-e2e").
 					WithQuery("listOptions.fieldSelector", "metadata.namespace="+nsName).
 					Expect().
-					Status(200).
-					JSON().
-					Path("$.items").
-					Null()
+					Status(403)
 			})
 
 			// Test get archived wf with good token
@@ -886,6 +883,7 @@ spec:
 	} {
 		s.Run(tt.name, func() {
 			path := s.e(s.T()).GET("/api/v1/archived-workflows").
+				WithQuery("listOptions.fieldSelector", "metadata.namespace=argo").
 				WithQuery("listOptions.labelSelector", tt.selector).
 				Expect().
 				Status(200).
@@ -905,6 +903,7 @@ spec:
 	s.Run("ListWithLimitAndOffset", func() {
 		j := s.e(s.T()).GET("/api/v1/archived-workflows").
 			WithQuery("listOptions.labelSelector", "argo-e2e").
+			WithQuery("listOptions.fieldSelector", "metadata.namespace=argo").
 			WithQuery("listOptions.limit", 1).
 			WithQuery("listOptions.offset", 1).
 			Expect().
@@ -921,15 +920,14 @@ spec:
 	})
 
 	s.Run("ListWithMinStartedAtGood", func() {
-		fieldSelector := "spec.startedAt>" + time.Now().Add(-1*time.Hour).Format(time.RFC3339) + ",spec.startedAt<" + time.Now().Add(1*time.Hour).Format(time.RFC3339)
-		j := s.e(s.T()).GET("/api/v1/archived-workflows").
+		fieldSelector := "metadata.namespace=argo,spec.startedAt>" + time.Now().Add(-1*time.Hour).Format(time.RFC3339) + ",spec.startedAt<" + time.Now().Add(1*time.Hour).Format(time.RFC3339)
+		s.e(s.T()).GET("/api/v1/archived-workflows").
 			WithQuery("listOptions.labelSelector", "argo-e2e").
 			WithQuery("listOptions.fieldSelector", fieldSelector).
 			WithQuery("listOptions.limit", 2).
 			Expect().
 			Status(200).
-			JSON()
-		j.
+			JSON().
 			Path("$.items").
 			Array().
 			Length().
@@ -937,14 +935,13 @@ spec:
 	})
 
 	s.Run("ListWithMinStartedAtBad", func() {
-		j := s.e(s.T()).GET("/api/v1/archived-workflows").
+		s.e(s.T()).GET("/api/v1/archived-workflows").
 			WithQuery("listOptions.labelSelector", "argo-e2e").
-			WithQuery("listOptions.fieldSelector", "spec.startedAt>"+time.Now().Add(1*time.Hour).Format(time.RFC3339)).
+			WithQuery("listOptions.fieldSelector", "metadata.namespace=argo,spec.startedAt>"+time.Now().Add(1*time.Hour).Format(time.RFC3339)).
 			WithQuery("listOptions.limit", 2).
 			Expect().
 			Status(200).
-			JSON()
-		j.
+			JSON().
 			Path("$.items").Null()
 	})
 
diff --git a/test/e2e/fixtures/e2e_suite.go b/test/e2e/fixtures/e2e_suite.go
index b5bd8722b..87fb10fcb 100644
--- a/test/e2e/fixtures/e2e_suite.go
+++ b/test/e2e/fixtures/e2e_suite.go
@@ -12,6 +12,7 @@ import (
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
 	"sigs.k8s.io/yaml"
@@ -100,12 +101,28 @@ func (s *E2ESuite) DeleteResources(label string) {
 		for _, wf := range list.Items {
 			isTestWf[wf.Name] = false
 			if s.Persistence.IsEnabled() && wf.Status.IsOffloadNodeStatus() {
-				// TODO - may make tests flakey
 				err := s.Persistence.offloadNodeStatusRepo.Delete(string(wf.UID), wf.Status.OffloadNodeStatusVersion)
 				if err != nil {
 					panic(err)
 				}
-				err = s.Persistence.workflowArchive.DeleteWorkflow(string(wf.UID))
+			}
+		}
+	}
+
+	// delete from the archive
+	{
+		if s.Persistence.IsEnabled() {
+			archive := s.Persistence.workflowArchive
+			parse, err := labels.ParseToRequirements(Label)
+			if err != nil {
+				panic(err)
+			}
+			workflows, err := archive.ListWorkflows(Namespace, time.Time{}, time.Time{}, parse, 0, 0)
+			if err != nil {
+				panic(err)
+			}
+			for _, workflow := range workflows {
+				err := archive.DeleteWorkflow(string(workflow.UID))
 				if err != nil {
 					panic(err)
 				}
@@ -115,7 +132,7 @@ func (s *E2ESuite) DeleteResources(label string) {
 
 	// delete all workflows
 	{
-		list, err := s.wfClient.List(metav1.ListOptions{LabelSelector: label})
+		list, err := s.wfClient.List(metav1.ListOptions{LabelSelector: Label})
 		if err != nil {
 			panic(err)
 		}
diff --git a/test/e2e/fixtures/given.go b/test/e2e/fixtures/given.go
index 33fdeea51..46141fc3f 100644
--- a/test/e2e/fixtures/given.go
+++ b/test/e2e/fixtures/given.go
@@ -33,6 +33,7 @@ type Given struct {
 // 1. A file name if it starts with "@"
 // 2. Raw YAML.
 func (g *Given) Workflow(text string) *Given {
+	g.t.Helper()
 	var file string
 	if strings.HasPrefix(text, "@") {
 		file = strings.TrimPrefix(text, "@")