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

Signed-off-by: Anton Gilgur <[email protected]> Signed-off-by: Alan Clucas <[email protected]> Co-authored-by: Anton Gilgur <[email protected]>
argoproj · Dec 2, 2024 · 97b94f0 · 97b94f0
1 parent 5163beb
commit 97b94f0
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 11 deletions.
diff --git a/server/workflow/workflow_server.go b/server/workflow/workflow_server.go
@@ -179,10 +179,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 {
@@ -692,7 +692,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)
 	}
@@ -714,22 +713,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)
 }

diff --git a/test/e2e/argo_server_test.go b/test/e2e/argo_server_test.go
@@ -479,6 +479,9 @@ func (s *ArgoServerSuite) TestPermission() {
 		badToken = string(secret.Data["token"])
 	})
 
+	// fake / spoofed token
+	fakeToken := "faketoken"
+
 	token := s.bearerToken
 	defer func() { s.bearerToken = token }()
 
@@ -582,8 +585,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() {
@@ -623,7 +626,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/"+nsName).
+			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/" + nsName).
+			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/" + nsName).
+			Expect().
+			Status(401)
+	})
+
 	// Test deleting archived wf with bad token
+	s.bearerToken = badToken
 	s.Run("DeleteArchivedWFsBadToken", func() {
 		s.e().DELETE("/api/v1/archived-workflows/" + uid).
 			Expect().