Merge commit from fork

* fix(api): properly authorize GET workflow fallback to archive

- the authorization was accidentally removed in f1ab5aa

- also if no archived workflow is found, properly return the original error as per ac9e2de

Signed-off-by: Anton Gilgur <[email protected]>

* test: add permission assertions for GET WF archived fallback

- the permission suite seemed to not have previously tested this
- add good, bad, and fake token checks
  - where "bad" is valid, but unauthorized, while "fake" is valid in format, but not corresponding to a real token

Signed-off-by: Anton Gilgur <[email protected]>

* fix(test): assign `goodToken` initially

- these tests were relying on ordering of assignments before, i.e. 1. good 2. bad 3. bad
  - should just assign before each test instead for simplicity / less complexity / less mistakes

Signed-off-by: Anton Gilgur <[email protected]>

---------

Signed-off-by: Anton Gilgur <[email protected]>
Co-authored-by: Anton Gilgur <[email protected]>

Author: Joibel

server/workflow/workflow_server.go

@@ -174,10 +174,10 @@ func (s *workflowServer) ListWorkflows(ctx context.Context, req *workflowpkg.Wor
 	s.instanceIDService.With(&listOption)
 
 	options, err := sutils.BuildListOptions(listOption, req.Namespace, "", req.NameFilter)
-
 	if err != nil {
 		return nil, err
 	}
+
 	// verify if we have permission to list Workflows
 	allowed, err := auth.CanI(ctx, "list", workflow.WorkflowPlural, options.Namespace, "")
 	if err != nil {
@@ -688,7 +688,6 @@ func (s *workflowServer) PodLogs(req *workflowpkg.WorkflowLogRequest, ws workflo
 	req.Name = wf.Name
 
 	err = ws.SendHeader(metadata.MD{})
-
 	if err != nil {
 		return sutils.ToStatusError(err, codes.Internal)
 	}
@@ -710,22 +709,34 @@ func (s *workflowServer) getWorkflow(ctx context.Context, wfClient versioned.Int
 		log.Debugf("Resolved alias %s to workflow %s.\n", latestAlias, latest.Name)
 		return latest, nil
 	}
-	var err error
+
 	wf, origErr := wfClient.ArgoprojV1alpha1().Workflows(namespace).Get(ctx, name, options)
+	// fallback to retrieve from archived workflows
 	if wf == nil || origErr != nil {
-		wf, err = s.wfArchive.GetWorkflow("", namespace, name)
+		allowed, err := auth.CanI(ctx, "get", workflow.WorkflowPlural, namespace, name)
 		if err != nil {
-			log.Errorf("failed to get live workflow: %v; failed to get archived workflow: %v", origErr, err)
-			// We only return the original error to preserve the original status code.
-			return nil, sutils.ToStatusError(origErr, codes.Internal)
+			return nil, getWorkflowOrigErr(origErr, err)
 		}
-		if wf == nil {
-			return nil, status.Error(codes.NotFound, "not found")
+		if !allowed {
+			err = status.Error(codes.PermissionDenied, "permission denied")
+			return nil, getWorkflowOrigErr(origErr, err)
+		}
+
+		wf, err = s.wfArchive.GetWorkflow("", namespace, name)
+		if wf == nil || err != nil {
+			return nil, getWorkflowOrigErr(origErr, err)
 		}
 	}
 	return wf, nil
 }
 
+// getWorkflowOrigErr only returns the original error to preserve the original status code
+// it logs out the new error
+func getWorkflowOrigErr(origErr error, err error) error {
+	log.Errorf("failed to get live workflow: %v; failed to get archived workflow: %v", origErr, err)
+	return sutils.ToStatusError(origErr, codes.Internal)
+}
+
 func (s *workflowServer) validateWorkflow(wf *wfv1.Workflow) error {
 	return sutils.ToStatusError(s.instanceIDService.Validate(wf), codes.InvalidArgument)
 }

test/e2e/argo_server_test.go

@@ -461,6 +461,9 @@ func (s *ArgoServerSuite) TestPermission() {
 		badToken = string(secret.Data["token"])
 	})
 
+	// fake / spoofed token
+	fakeToken := "faketoken"
+
 	token := s.bearerToken
 	defer func() { s.bearerToken = token }()
 
@@ -564,8 +567,8 @@ func (s *ArgoServerSuite) TestPermission() {
 			Status(200)
 	})
 
-	// we've now deleted the workflow, but it is still in the archive, testing the archive
-	// after deleting the workflow makes sure that we are no dependant of the workflow for authorization
+	// we've now deleted the workflow, but it is still in the archive
+	// testing the archive after deleting it makes sure that we are not dependent on a live workflow resource for authorization
 
 	// Test list archived WFs with good token
 	s.Run("ListArchivedWFsGoodToken", func() {
@@ -605,7 +608,33 @@ func (s *ArgoServerSuite) TestPermission() {
 			Status(403)
 	})
 
+	// Test get wf w/ archive fallback with good token
+	s.bearerToken = goodToken
+	s.Run("GetWFsFallbackArchivedGoodToken", func() {
+		s.e().GET("/api/v1/workflows/"+uid).
+			WithQuery("listOptions.labelSelector", "workflows.argoproj.io/test").
+			Expect().
+			Status(200)
+	})
+
+	// Test get wf w/ archive fallback with bad token
+	s.bearerToken = badToken
+	s.Run("GetWFsFallbackArchivedBadToken", func() {
+		s.e().GET("/api/v1/workflows/" + uid).
+			Expect().
+			Status(403)
+	})
+
+	// Test get wf w/ archive fallback with fake token
+	s.bearerToken = fakeToken
+	s.Run("GetWFsFallbackArchivedFakeToken", func() {
+		s.e().GET("/api/v1/workflows/" + uid).
+			Expect().
+			Status(403)
+	})
+
 	// Test deleting archived wf with bad token
+	s.bearerToken = badToken
 	s.Run("DeleteArchivedWFsBadToken", func() {
 		s.e().DELETE("/api/v1/archived-workflows/" + uid).
 			Expect().