From 53d322ec69ff71f7ed5f7c96a0b6264fbf1e8097 Mon Sep 17 00:00:00 2001 From: Sander van Harmelen Date: Thu, 4 Oct 2018 20:35:41 +0200 Subject: [PATCH 1/2] Test lock timeout errors when running a plan --- backend/remote/backend_plan_test.go | 72 +++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/backend/remote/backend_plan_test.go b/backend/remote/backend_plan_test.go index c005e2c33..2d08ad1f1 100644 --- a/backend/remote/backend_plan_test.go +++ b/backend/remote/backend_plan_test.go @@ -2,8 +2,12 @@ package remote import ( "context" + "os" + "os/signal" "strings" + "syscall" "testing" + "time" tfe "github.com/hashicorp/go-tfe" "github.com/hashicorp/terraform/backend" @@ -140,6 +144,74 @@ func TestRemote_planNoConfig(t *testing.T) { } } +func TestRemote_planLockTimeout(t *testing.T) { + b := testBackendDefault(t) + ctx := context.Background() + + // Retrieve the workspace used to run this operation in. + w, err := b.client.Workspaces.Read(ctx, b.organization, b.workspace) + if err != nil { + t.Fatalf("error retrieving workspace: %v", err) + } + + // Create a new configuration version. + c, err := b.client.ConfigurationVersions.Create(ctx, w.ID, tfe.ConfigurationVersionCreateOptions{}) + if err != nil { + t.Fatalf("error creating configuration version: %v", err) + } + + // Create a pending run to block this run. + _, err = b.client.Runs.Create(ctx, tfe.RunCreateOptions{ + ConfigurationVersion: c, + Workspace: w, + }) + if err != nil { + t.Fatalf("error creating pending run: %v", err) + } + + mod, modCleanup := module.TestTree(t, "./test-fixtures/plan") + defer modCleanup() + + input := testInput(t, map[string]string{ + "cancel": "yes", + "approve": "yes", + }) + + op := testOperationPlan() + op.StateLockTimeout = 5 * time.Second + op.Module = mod + op.UIIn = input + op.UIOut = b.CLI + op.Workspace = backend.DefaultStateName + + _, err = b.Operation(context.Background(), op) + if err != nil { + t.Fatalf("error starting operation: %v", err) + } + + sigint := make(chan os.Signal, 1) + signal.Notify(sigint, syscall.SIGINT) + select { + case <-sigint: + // Stop redirecting SIGINT signals. + signal.Stop(sigint) + case <-time.After(10 * time.Second): + t.Fatalf("expected lock timeout after 5 seconds, waited 10 seconds") + } + + if len(input.answers) != 2 { + t.Fatalf("expected unused answers, got: %v", input.answers) + } + + output := b.CLI.(*cli.MockUi).OutputWriter.String() + if !strings.Contains(output, "Lock timeout exceeded") { + t.Fatalf("missing lock timout error in output: %s", output) + } + if strings.Contains(output, "1 to add, 0 to change, 0 to destroy") { + t.Fatalf("unexpected plan summery in output: %s", output) + } +} + func TestRemote_planDestroy(t *testing.T) { b := testBackendDefault(t) From ffc67a8e90a9c9cc3389c21d765a03d38d016f73 Mon Sep 17 00:00:00 2001 From: Sander van Harmelen Date: Thu, 4 Oct 2018 22:53:56 +0200 Subject: [PATCH 2/2] Prevent running plan or apply without permissions --- backend/remote/backend_apply.go | 32 ++++++++++++++++++----- backend/remote/backend_apply_test.go | 39 +++++++++++++++++++++++++++- backend/remote/backend_mock.go | 4 +++ backend/remote/backend_plan.go | 23 +++++++++++----- backend/remote/backend_plan_test.go | 37 ++++++++++++++++++++++++++ 5 files changed, 121 insertions(+), 14 deletions(-) diff --git a/backend/remote/backend_apply.go b/backend/remote/backend_apply.go index a260138fb..df5994f56 100644 --- a/backend/remote/backend_apply.go +++ b/backend/remote/backend_apply.go @@ -22,6 +22,11 @@ func (b *Remote) opApply(stopCtx, cancelCtx context.Context, op *backend.Operati return nil, generalError("error retrieving workspace", err) } + if !w.Permissions.CanUpdate { + return nil, fmt.Errorf(strings.TrimSpace( + fmt.Sprintf(applyErrNoUpdateRights, b.hostname, b.organization, op.Workspace))) + } + if w.VCSRepo != nil { return nil, fmt.Errorf(strings.TrimSpace(applyErrVCSNotSupported)) } @@ -76,6 +81,9 @@ func (b *Remote) opApply(stopCtx, cancelCtx context.Context, op *backend.Operati return r, nil } + // Since we already checked the permissions before creating the run + // this should never happen. But it doesn't hurt to keep this in as + // a safeguard for any unexpected situations. if !r.Permissions.CanApply { // Make sure we discard the run if possible. if r.Actions.IsDiscardable { @@ -261,10 +269,20 @@ func (b *Remote) confirm(stopCtx context.Context, op *backend.Operation, opts *t return nil } -const applyErrVCSNotSupported = ` -Apply not allowed for workspaces with a VCS connection! +const applyErrNoUpdateRights = ` +Insufficient rights to apply changes! -A workspace that is connected to a VCS requires the VCS based workflow +[reset][yellow]The provided credentials have insufficient rights to apply changes. In order +to apply changes at least write permissions on the workspace are required. To +queue a run that can be approved by someone else, please use the 'Queue Plan' +button in the web UI: +https://%s/app/%s/%s/runs[reset] +` + +const applyErrVCSNotSupported = ` +Apply not allowed for workspaces with a VCS connection. + +A workspace that is connected to a VCS requires the VCS-driven workflow to ensure that the VCS remains the single source of truth. ` @@ -293,10 +311,10 @@ does not require any configuration files. const applyErrNoApplyRights = ` Insufficient rights to approve the pending changes! -[reset][yellow]There are pending changes, but the used credentials have insufficient rights -to approve them. The run will be discarded to prevent it from blocking the -queue waiting for external approval. To trigger a run that can be approved by -someone else, please use the 'Queue Plan' button in the web UI: +[reset][yellow]There are pending changes, but the provided credentials have insufficient rights +to approve them. The run will be discarded to prevent it from blocking the queue +waiting for external approval. To queue a run that can be approved by someone +else, please use the 'Queue Plan' button in the web UI: https://%s/app/%s/%s/runs[reset] ` diff --git a/backend/remote/backend_apply_test.go b/backend/remote/backend_apply_test.go index 1857141e3..998e323bc 100644 --- a/backend/remote/backend_apply_test.go +++ b/backend/remote/backend_apply_test.go @@ -61,10 +61,47 @@ func TestRemote_applyBasic(t *testing.T) { } } +func TestRemote_applyWithoutPermissions(t *testing.T) { + b := testBackendNoDefault(t) + + // Create a named workspace without permissions. + w, err := b.client.Workspaces.Create( + context.Background(), + b.organization, + tfe.WorkspaceCreateOptions{ + Name: tfe.String(b.prefix + "prod"), + }, + ) + if err != nil { + t.Fatalf("error creating named workspace: %v", err) + } + w.Permissions.CanUpdate = false + + mod, modCleanup := module.TestTree(t, "./test-fixtures/apply") + defer modCleanup() + + op := testOperationApply() + op.Module = mod + op.Workspace = "prod" + + run, err := b.Operation(context.Background(), op) + if err != nil { + t.Fatalf("error starting operation: %v", err) + } + <-run.Done() + + if run.Err == nil { + t.Fatalf("expected an apply error, got: %v", run.Err) + } + if !strings.Contains(run.Err.Error(), "insufficient rights to apply changes") { + t.Fatalf("expected a permissions error, got: %v", run.Err) + } +} + func TestRemote_applyWithVCS(t *testing.T) { b := testBackendNoDefault(t) - // Create the named workspace with a VCS. + // Create a named workspace with a VCS. _, err := b.client.Workspaces.Create( context.Background(), b.organization, diff --git a/backend/remote/backend_mock.go b/backend/remote/backend_mock.go index 55011aad6..93fd2558e 100644 --- a/backend/remote/backend_mock.go +++ b/backend/remote/backend_mock.go @@ -853,6 +853,10 @@ func (m *mockWorkspaces) Create(ctx context.Context, organization string, option w := &tfe.Workspace{ ID: generateID("ws-"), Name: *options.Name, + Permissions: &tfe.WorkspacePermissions{ + CanQueueRun: true, + CanUpdate: true, + }, } if options.VCSRepo != nil { w.VCSRepo = &tfe.VCSRepo{} diff --git a/backend/remote/backend_plan.go b/backend/remote/backend_plan.go index aec4fe9e3..df7d76fac 100644 --- a/backend/remote/backend_plan.go +++ b/backend/remote/backend_plan.go @@ -20,6 +20,16 @@ import ( func (b *Remote) opPlan(stopCtx, cancelCtx context.Context, op *backend.Operation) (*tfe.Run, error) { log.Printf("[INFO] backend/remote: starting Plan operation") + // Retrieve the workspace used to run this operation in. + w, err := b.client.Workspaces.Read(stopCtx, b.organization, op.Workspace) + if err != nil { + return nil, generalError("error retrieving workspace", err) + } + + if !w.Permissions.CanQueueRun { + return nil, fmt.Errorf(strings.TrimSpace(fmt.Sprintf(planErrNoQueueRunRights))) + } + if op.Plan != nil { return nil, fmt.Errorf(strings.TrimSpace(planErrPlanNotSupported)) } @@ -36,12 +46,6 @@ func (b *Remote) opPlan(stopCtx, cancelCtx context.Context, op *backend.Operatio return nil, fmt.Errorf(strings.TrimSpace(planErrNoConfig)) } - // Retrieve the workspace used to run this operation in. - w, err := b.client.Workspaces.Read(stopCtx, b.organization, op.Workspace) - if err != nil { - return nil, generalError("error retrieving workspace", err) - } - return b.plan(stopCtx, cancelCtx, op, w) } @@ -192,6 +196,13 @@ func (b *Remote) plan(stopCtx, cancelCtx context.Context, op *backend.Operation, return r, nil } +const planErrNoQueueRunRights = ` +Insufficient rights to generate a plan! + +[reset][yellow]The provided credentials have insufficient rights to generate a plan. In order +to generate plans, at least plan permissions on the workspace are required.[reset] +` + const planErrPlanNotSupported = ` Displaying a saved plan is currently not supported! diff --git a/backend/remote/backend_plan_test.go b/backend/remote/backend_plan_test.go index 2d08ad1f1..3718b6045 100644 --- a/backend/remote/backend_plan_test.go +++ b/backend/remote/backend_plan_test.go @@ -48,6 +48,43 @@ func TestRemote_planBasic(t *testing.T) { } } +func TestRemote_planWithoutPermissions(t *testing.T) { + b := testBackendNoDefault(t) + + // Create a named workspace without permissions. + w, err := b.client.Workspaces.Create( + context.Background(), + b.organization, + tfe.WorkspaceCreateOptions{ + Name: tfe.String(b.prefix + "prod"), + }, + ) + if err != nil { + t.Fatalf("error creating named workspace: %v", err) + } + w.Permissions.CanQueueRun = false + + mod, modCleanup := module.TestTree(t, "./test-fixtures/plan") + defer modCleanup() + + op := testOperationPlan() + op.Module = mod + op.Workspace = "prod" + + run, err := b.Operation(context.Background(), op) + if err != nil { + t.Fatalf("error starting operation: %v", err) + } + <-run.Done() + + if run.Err == nil { + t.Fatalf("expected a plan error, got: %v", run.Err) + } + if !strings.Contains(run.Err.Error(), "insufficient rights to generate a plan") { + t.Fatalf("expected a permissions error, got: %v", run.Err) + } +} + func TestRemote_planWithPlan(t *testing.T) { b := testBackendDefault(t)