diff --git a/server/auth/authorizer.go b/server/auth/authorizer.go index bee70ab2d97c..51d167eda175 100644 --- a/server/auth/authorizer.go +++ b/server/auth/authorizer.go @@ -2,10 +2,11 @@ package auth import ( "context" - "sort" 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) { @@ -49,9 +50,9 @@ func (a Authorizer) CanI(verb, resource, namespace, name string) (bool, error) { for _, rule := range a.status[namespace].ResourceRules { if allowed(rule.Verbs, verb) && allowed(rule.Resources, resource) && - allowed(rule.APIGroups, "argoproj.io") && + allowed(rule.APIGroups, workflow.Group) && allowed(rule.ResourceNames, name) { - logCtx.WithField("allowed", true).Debug("CanI") + logCtx.WithFields(log.Fields{"rule": rule, "allowed": true}).Debug("CanI") return true, nil } } @@ -64,5 +65,14 @@ func NewAuthorizer(ctx context.Context) *Authorizer { } func allowed(values []string, value string) bool { - return len(values) == 0 || sort.SearchStrings(values, "*") >= 0 || sort.SearchStrings(values, value) >= 0 + 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 652159714d28..682f76bfc575 100644 --- a/server/workflowarchive/archived_workflow_server.go +++ b/server/workflowarchive/archived_workflow_server.go @@ -12,6 +12,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/argoproj/argo/persist/sqldb" + "github.com/argoproj/argo/pkg/apis/workflow" wfv1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1" "github.com/argoproj/argo/server/auth" ) @@ -57,7 +58,7 @@ func (w *archivedWorkflowServer) ListArchivedWorkflows(ctx context.Context, req return nil, err } for _, wf := range moreItems { - allowed, err := authorizer.CanI("get", "workflow", wf.Namespace, wf.Name) + allowed, err := authorizer.CanI("get", workflow.WorkflowPlural, wf.Namespace, wf.Name) if err != nil { return nil, err } @@ -86,7 +87,7 @@ func (w *archivedWorkflowServer) GetArchivedWorkflow(ctx context.Context, req *G if wf == nil { return nil, status.Error(codes.NotFound, "not found") } - allowed, err := auth.CanI(ctx, "get", "workflows", wf.Namespace, wf.Name) + allowed, err := auth.CanI(ctx, "get", workflow.WorkflowPlural, wf.Namespace, wf.Name) if err != nil { return nil, err } @@ -101,7 +102,7 @@ func (w *archivedWorkflowServer) DeleteArchivedWorkflow(ctx context.Context, req if err != nil { return nil, err } - allowed, err := auth.CanI(ctx, "delete", "workflows", wf.Namespace, wf.Name) + allowed, err := auth.CanI(ctx, "delete", workflow.WorkflowPlural, wf.Namespace, wf.Name) if err != nil { return nil, err } diff --git a/test/e2e/argo_server_test.go b/test/e2e/argo_server_test.go index e61c66e78f83..f6b9badfe072 100644 --- a/test/e2e/argo_server_test.go +++ b/test/e2e/argo_server_test.go @@ -222,7 +222,7 @@ func (s *ArgoServerSuite) TestPermission() { Path("$.items"). Array(). Length(). - Gt(0) + Equal(1) }) // Test creating workflow with bad token @@ -265,14 +265,31 @@ func (s *ArgoServerSuite) TestPermission() { Status(403) }) - if s.Persistence.IsEnabled() { + s.Given(). + WorkflowName("test-wf-good"). + When(). + WaitForWorkflow(30 * time.Second) - // Simply wait 10 seconds for the wf to be completed - s.Given(). - WorkflowName("test-wf-good"). - When(). - WaitForWorkflow(10 * time.Second) + // Test delete workflow with bad token + s.bearerToken = badToken + s.Run("DeleteWFWithBadToken", func(t *testing.T) { + s.e(t).DELETE("/api/v1/workflows/" + nsName + "/test-wf-good"). + Expect(). + Status(403) + }) + + // Test delete workflow with good token + s.bearerToken = goodToken + s.Run("DeleteWFWithGoodToken", func(t *testing.T) { + s.e(t).DELETE("/api/v1/workflows/" + nsName + "/test-wf-good"). + Expect(). + 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 + if s.Persistence.IsEnabled() { // Test list archived WFs with good token s.bearerToken = goodToken s.Run("ListArchivedWFsGoodToken", func(t *testing.T) { @@ -286,61 +303,50 @@ func (s *ArgoServerSuite) TestPermission() { Array().Length().Gt(0) }) + s.bearerToken = badToken + s.Run("ListArchivedWFsBadToken", func(t *testing.T) { + s.e(t).GET("/api/v1/archived-workflows"). + WithQuery("listOptions.labelSelector", "argo-e2e"). + WithQuery("listOptions.fieldSelector", "metadata.namespace="+nsName). + Expect(). + Status(200). + JSON(). + Path("$.items"). + Null() + }) + // Test get archived wf with good token s.bearerToken = goodToken s.Run("GetArchivedWFsGoodToken", func(t *testing.T) { s.e(t).GET("/api/v1/archived-workflows/"+uid). WithQuery("listOptions.labelSelector", "argo-e2e"). Expect(). - Status(200). - JSON(). - Path("$.metadata.name"). - Equal("test-wf-good") + Status(200) }) - // Test list archived WFs with bad token - // TODO: Uncomment following code after https://github.com/argoproj/argo/issues/2049 is resolved. - - // s.bearerToken = badToken - // s.Run("ListArchivedWFsBadToken", func(t *testing.T) { - // s.e(t).GET("/api/v1/archived-workflows"). - // WithQuery("listOptions.labelSelector", "argo-e2e"). - // WithQuery("listOptions.fieldSelector", "metadata.namespace="+nsName). - // Expect(). - // Status(200). - // JSON(). - // Path("$.items"). - // Array(). - // Length(). - // Equal(0) - // }) - // Test get archived wf with bad token s.bearerToken = badToken - s.Run("ListArchivedWFsBadToken", func(t *testing.T) { - s.e(t).GET("/api/v1/archived-workflows/"+uid). - WithQuery("listOptions.labelSelector", "argo-e2e"). + s.Run("GetArchivedWFsBadToken", func(t *testing.T) { + s.e(t).GET("/api/v1/archived-workflows/" + uid). Expect(). Status(403) }) + // Test deleting archived wf with bad token + s.bearerToken = badToken + s.Run("DeleteArchivedWFsBadToken", func(t *testing.T) { + s.e(t).DELETE("/api/v1/archived-workflows/" + uid). + Expect(). + Status(403) + }) + // Test deleting archived wf with good token + s.bearerToken = goodToken + s.Run("DeleteArchivedWFsGoodToken", func(t *testing.T) { + s.e(t).DELETE("/api/v1/archived-workflows/" + uid). + Expect(). + Status(200) + }) } - - // Test delete workflow with bad token - s.bearerToken = badToken - s.Run("DeleteWFWithBadToken", func(t *testing.T) { - s.e(t).DELETE("/api/v1/workflows/" + nsName + "/test-wf-good"). - Expect(). - Status(403) - }) - - // Test delete workflow with good token - s.bearerToken = goodToken - s.Run("DeleteWFWithGoodToken", func(t *testing.T) { - s.e(t).DELETE("/api/v1/workflows/" + nsName + "/test-wf-good"). - Expect(). - Status(200) - }) } func (s *ArgoServerSuite) TestLintWorkflow() {