cli: remove hard requirement on list-jobs (#16380)

Most job subcommands allow for job ID prefix match as a convenience functionality so users don't have to type the full job ID. But this introduces a hard ACL requirement that the token used to run these commands have the `list-jobs` permission, even if the token has enough permission to execute the basic command action and the user passed an exact job ID. This change softens this requirement by not failing the prefix match in case the request results in a permission denied error and instead using the information passed by the user directly.
hashicorp · Mar 9, 2023 · 4fdb5c4 · 4fdb5c4
1 parent 1dd3203
commit 4fdb5c4
Show file tree

Hide file tree

Showing 42 changed files with 1,631 additions and 47 deletions.
diff --git a/.changelog/16380.txt b/.changelog/16380.txt
@@ -0,0 +1,3 @@
+``release-note:improvement
+cli: Remove requirement for `list-jobs` capability on several job subcommands that didn't strictly needed it
+```
diff --git a/command/job_allocs.go b/command/job_allocs.go
@@ -19,8 +19,9 @@ Usage: nomad job allocs [options] <job>
 
   Display allocations for a particular job.
 
-  When ACLs are enabled, this command requires a token with the 'read-job' and
-  'list-jobs' capabilities for the job's namespace.
+  When ACLs are enabled, this command requires a token with the 'read-job'
+  capability for the job's namespace. The 'list-jobs' capability is required to
+  run the command with a job prefix instead of the exact job ID.
 
 General Options:
 

diff --git a/command/job_allocs_test.go b/command/job_allocs_test.go
@@ -3,11 +3,14 @@ package command
 import (
 	"testing"
 
+	"github.com/hashicorp/nomad/api"
 	"github.com/hashicorp/nomad/ci"
+	"github.com/hashicorp/nomad/command/agent"
 	"github.com/hashicorp/nomad/nomad/mock"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/mitchellh/cli"
 	"github.com/posener/complete"
+	"github.com/shoenig/test/must"
 	"github.com/stretchr/testify/require"
 )
 
@@ -172,3 +175,116 @@ func TestJobAllocsCommand_AutocompleteArgs(t *testing.T) {
 	require.Equal(t, 1, len(res))
 	require.Equal(t, j.ID, res[0])
 }
+
+func TestJobAllocsCommand_ACL(t *testing.T) {
+	ci.Parallel(t)
+
+	// Start server with ACL enabled.
+	srv, _, url := testServer(t, true, func(c *agent.Config) {
+		c.ACL.Enabled = true
+	})
+	defer srv.Shutdown()
+
+	// Create a job with an alloc.
+	job := mock.Job()
+	state := srv.Agent.Server().State()
+	err := state.UpsertJob(structs.MsgTypeTestSetup, 100, job)
+	must.NoError(t, err)
+
+	a := mock.Alloc()
+	a.Job = job
+	a.JobID = job.ID
+	a.TaskGroup = job.TaskGroups[0].Name
+	a.Metrics = &structs.AllocMetric{}
+	a.DesiredStatus = structs.AllocDesiredStatusRun
+	a.ClientStatus = structs.AllocClientStatusRunning
+	err = state.UpsertAllocs(structs.MsgTypeTestSetup, 200, []*structs.Allocation{a})
+	must.NoError(t, err)
+
+	testCases := []struct {
+		name        string
+		jobPrefix   bool
+		aclPolicy   string
+		expectedErr string
+		expectedOut string
+	}{
+		{
+			name:        "no token",
+			aclPolicy:   "",
+			expectedErr: api.PermissionDeniedErrorContent,
+		},
+		{
+			name: "missing read-job",
+			aclPolicy: `
+namespace "default" {
+	capabilities = ["alloc-lifecycle"]
+}
+`,
+			expectedErr: api.PermissionDeniedErrorContent,
+		},
+		{
+			name: "read-job allowed",
+			aclPolicy: `
+namespace "default" {
+	capabilities = ["read-job"]
+}
+`,
+		},
+		{
+			name:      "job prefix requires list-job",
+			jobPrefix: true,
+			aclPolicy: `
+namespace "default" {
+	capabilities = ["read-job"]
+}
+`,
+			expectedOut: "No allocations",
+		},
+		{
+			name:      "job prefix works with list-job",
+			jobPrefix: true,
+			aclPolicy: `
+namespace "default" {
+	capabilities = ["read-job", "list-jobs"]
+}
+`,
+		},
+	}
+
+	for i, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			ui := cli.NewMockUi()
+			cmd := &JobAllocsCommand{Meta: Meta{Ui: ui}}
+			args := []string{
+				"-address", url,
+			}
+
+			if tc.aclPolicy != "" {
+				// Create ACL token with test case policy and add it to the
+				// command.
+				policyName := nonAlphaNum.ReplaceAllString(tc.name, "-")
+				token := mock.CreatePolicyAndToken(t, state, uint64(302+i), policyName, tc.aclPolicy)
+				args = append(args, "-token", token.SecretID)
+			}
+
+			// Add job ID or job ID prefix to the command.
+			if tc.jobPrefix {
+				args = append(args, job.ID[:3])
+			} else {
+				args = append(args, job.ID)
+			}
+
+			// Run command.
+			code := cmd.Run(args)
+			if tc.expectedErr == "" {
+				must.Zero(t, code)
+			} else {
+				must.One(t, code)
+				must.StrContains(t, ui.ErrorWriter.String(), tc.expectedErr)
+			}
+			if tc.expectedOut != "" {
+				must.StrContains(t, ui.OutputWriter.String(), tc.expectedOut)
+			}
+		})
+	}
+}
diff --git a/command/job_deployments.go b/command/job_deployments.go
@@ -19,8 +19,9 @@ Usage: nomad job deployments [options] <job>
 
   Deployments is used to display the deployments for a particular job.
 
-  When ACLs are enabled, this command requires a token with the 'read-job' and
-  'list-jobs' capabilities for the job's namespace.
+  When ACLs are enabled, this command requires a token with the 'read-job'
+  capability for the job's namespace. The 'list-jobs' capability is required to
+  run the command with a job prefix instead of the exact job ID.
 
 General Options:
 

diff --git a/command/job_deployments_test.go b/command/job_deployments_test.go
@@ -4,11 +4,14 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/hashicorp/nomad/api"
 	"github.com/hashicorp/nomad/ci"
+	"github.com/hashicorp/nomad/command/agent"
 	"github.com/hashicorp/nomad/nomad/mock"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/mitchellh/cli"
 	"github.com/posener/complete"
+	"github.com/shoenig/test/must"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -152,3 +155,112 @@ func TestJobDeploymentsCommand_AutocompleteArgs(t *testing.T) {
 	assert.Equal(1, len(res))
 	assert.Equal(j.ID, res[0])
 }
+
+func TestJobDeploymentsCommand_ACL(t *testing.T) {
+	ci.Parallel(t)
+
+	// Start server with ACL enabled.
+	srv, _, url := testServer(t, true, func(c *agent.Config) {
+		c.ACL.Enabled = true
+	})
+	defer srv.Shutdown()
+
+	// Create a job with a deployment.
+	job := mock.Job()
+	state := srv.Agent.Server().State()
+	err := state.UpsertJob(structs.MsgTypeTestSetup, 100, job)
+	must.NoError(t, err)
+
+	d := mock.Deployment()
+	d.JobID = job.ID
+	d.JobCreateIndex = job.CreateIndex
+	err = state.UpsertDeployment(101, d)
+	must.NoError(t, err)
+
+	testCases := []struct {
+		name        string
+		jobPrefix   bool
+		aclPolicy   string
+		expectedErr string
+		expectedOut string
+	}{
+		{
+			name:        "no token",
+			aclPolicy:   "",
+			expectedErr: api.PermissionDeniedErrorContent,
+		},
+		{
+			name: "missing read-job",
+			aclPolicy: `
+namespace "default" {
+	capabilities = ["alloc-lifecycle"]
+}
+`,
+			expectedErr: api.PermissionDeniedErrorContent,
+		},
+		{
+			name: "read-job allowed",
+			aclPolicy: `
+namespace "default" {
+	capabilities = ["read-job"]
+}
+`,
+		},
+		{
+			name:      "job prefix requires list-job",
+			jobPrefix: true,
+			aclPolicy: `
+namespace "default" {
+	capabilities = ["read-job"]
+}
+`,
+			expectedOut: "No deployments",
+		},
+		{
+			name:      "job prefix works with list-job",
+			jobPrefix: true,
+			aclPolicy: `
+namespace "default" {
+	capabilities = ["read-job", "list-jobs"]
+}
+`,
+		},
+	}
+
+	for i, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			ui := cli.NewMockUi()
+			cmd := &JobDeploymentsCommand{Meta: Meta{Ui: ui}}
+			args := []string{
+				"-address", url,
+			}
+
+			if tc.aclPolicy != "" {
+				// Create ACL token with test case policy and add it to the
+				// command.
+				policyName := nonAlphaNum.ReplaceAllString(tc.name, "-")
+				token := mock.CreatePolicyAndToken(t, state, uint64(302+i), policyName, tc.aclPolicy)
+				args = append(args, "-token", token.SecretID)
+			}
+
+			// Add job ID or job ID prefix to the command.
+			if tc.jobPrefix {
+				args = append(args, job.ID[:3])
+			} else {
+				args = append(args, job.ID)
+			}
+
+			// Run command.
+			code := cmd.Run(args)
+			if tc.expectedErr == "" {
+				must.Zero(t, code)
+			} else {
+				must.One(t, code)
+				must.StrContains(t, ui.ErrorWriter.String(), tc.expectedErr)
+			}
+			if tc.expectedOut != "" {
+				must.StrContains(t, ui.OutputWriter.String(), tc.expectedOut)
+			}
+		})
+	}
+}
diff --git a/command/job_dispatch.go b/command/job_dispatch.go
@@ -33,7 +33,10 @@ Usage: nomad job dispatch [options] <parameterized job> [input source]
   detach flag.
 
   When ACLs are enabled, this command requires a token with the 'dispatch-job'
-  capability for the job's namespace.
+  capability for the job's namespace. The 'list-jobs' capability is required to
+  run the command with a job prefix instead of the exact job ID. The 'read-job'
+  capability is required to monitor the resulting evaluation when -detach is
+  not used.
 
 General Options: