fix: have terraform output adhere to authorization w/ cloud

Normally, `terraform output` refreshes and reads the entire state in the command package before pulling output values out of it. This doesn't give Terraform Cloud the opportunity to apply the read state outputs org permission and instead applies the read state versions permission.

I decided to expand the state manager interface to provide a separate GetRootOutputValues function in order to give the cloud backend a more nuanced opportunity to fetch just the outputs. This required moving state Refresh/Read code that was previously in the command into the shared backend state as well as the filesystem state packages.

internal/backend/local/backend_local_test.go

@@ -220,6 +220,10 @@ func (s *stateStorageThatFailsRefresh) State() *states.State {
 	return nil
 }
 
+func (s *stateStorageThatFailsRefresh) GetRootOutputValues() (map[string]*states.OutputValue, error) {
+	return nil, fmt.Errorf("unimplemented")
+}
+
 func (s *stateStorageThatFailsRefresh) WriteState(*states.State) error {
 	return fmt.Errorf("unimplemented")
 }

internal/cloud/backend.go

@@ -19,7 +19,6 @@ import (
 	"github.com/hashicorp/terraform/internal/backend"
 	"github.com/hashicorp/terraform/internal/configs/configschema"
 	"github.com/hashicorp/terraform/internal/plans"
-	"github.com/hashicorp/terraform/internal/states/remote"
 	"github.com/hashicorp/terraform/internal/states/statemgr"
 	"github.com/hashicorp/terraform/internal/terraform"
 	"github.com/hashicorp/terraform/internal/tfdiags"
@@ -628,7 +627,7 @@ func (b *Cloud) StateMgr(name string) (statemgr.Full, error) {
 		runID: os.Getenv("TFE_RUN_ID"),
 	}
 
-	return &remote.State{Client: client}, nil
+	return NewState(client), nil
 }
 
 // Operation implements backend.Enhanced.

internal/cloud/backend_state_test.go

@@ -78,7 +78,7 @@ func TestRemoteClient_TestRemoteLocks(t *testing.T) {
 		t.Fatalf("expected no error, got %v", err)
 	}
 
-	remote.TestRemoteLocks(t, s1.(*remote.State).Client, s2.(*remote.State).Client)
+	remote.TestRemoteLocks(t, s1.(*State).Client, s2.(*State).Client)
 }
 
 func TestRemoteClient_withRunID(t *testing.T) {

internal/cloud/state.go

@@ -0,0 +1,110 @@
+package cloud
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+
+	"github.com/hashicorp/terraform/internal/states"
+	"github.com/hashicorp/terraform/internal/states/remote"
+	"github.com/hashicorp/terraform/internal/states/statemgr"
+	"github.com/zclconf/go-cty/cty"
+	"github.com/zclconf/go-cty/cty/gocty"
+)
+
+// State is similar to remote State and delegates to it, except in the case of output values,
+// which use a separate methodology that ensures the caller is authorized to read cloud
+// workspace outputs.
+type State struct {
+	Client *remoteClient
+
+	delegate remote.State
+}
+
+// Proof that cloud State is a statemgr.Persistent interface
+var _ statemgr.Persistent = (*State)(nil)
+
+func NewState(client *remoteClient) *State {
+	return &State{
+		Client:   client,
+		delegate: remote.State{Client: client},
+	}
+}
+
+// State delegates calls to read State to the remote State
+func (s *State) State() *states.State {
+	return s.delegate.State()
+}
+
+// Lock delegates calls to lock state to the remote State
+func (s *State) Lock(info *statemgr.LockInfo) (string, error) {
+	return s.delegate.Lock(info)
+}
+
+// Unlock delegates calls to unlock state to the remote State
+func (s *State) Unlock(id string) error {
+	return s.delegate.Unlock(id)
+}
+
+// RefreshState delegates calls to refresh State to the remote State
+func (s *State) RefreshState() error {
+	return s.delegate.RefreshState()
+}
+
+// RefreshState delegates calls to refresh State to the remote State
+func (s *State) PersistState() error {
+	return s.delegate.PersistState()
+}
+
+// WriteState delegates calls to write State to the remote State
+func (s *State) WriteState(state *states.State) error {
+	return s.delegate.WriteState(state)
+}
+
+// GetRootOutputValues fetches output values from Terraform Cloud
+func (s *State) GetRootOutputValues() (map[string]*states.OutputValue, error) {
+	ctx := context.Background()
+
+	so, err := s.Client.client.StateVersionOutputs.ReadCurrent(ctx, s.Client.workspace.ID)
+
+	if err != nil {
+		return nil, fmt.Errorf("Could not read state version outputs: %w", err)
+	}
+
+	result := make(map[string]*states.OutputValue)
+
+	for _, output := range so.Items {
+		if output.Sensitive {
+			// Since this is a sensitive value, the output must be requested explicitly in order to
+			// read its value, which is assumed to be present by callers
+			sensitiveOutput, err := s.Client.client.StateVersionOutputs.Read(ctx, output.ID)
+			if err != nil {
+				return nil, fmt.Errorf("could not read state version output %s: %w", output.ID, err)
+			}
+			output.Value = sensitiveOutput.Value
+		}
+
+		bufType, err := json.Marshal(output.DetailedType)
+		if err != nil {
+			return nil, fmt.Errorf("could not marshal output %s type: %w", output.ID, err)
+		}
+
+		var ctype cty.Type
+		err = ctype.UnmarshalJSON(bufType)
+		if err != nil {
+			return nil, fmt.Errorf("could not interpret output %s type: %w", output.ID, err)
+		}
+
+		cval, err := gocty.ToCtyValue(output.Value, ctype)
+		if err != nil {
+			return nil, fmt.Errorf("could not interpret value %v as type %s for output %s: %w", cval, ctype.FriendlyName(), output.ID, err)
+		}
+
+		result[output.Name] = &states.OutputValue{
+			Value:     cval,
+			Sensitive: output.Sensitive,
+		}
+	}
+
+	return result, nil
+}

internal/cloud/state_test.go

@@ -0,0 +1,82 @@
+package cloud
+
+import (
+	"testing"
+
+	"github.com/hashicorp/go-tfe"
+	"github.com/hashicorp/terraform/internal/states/statemgr"
+)
+
+func TestState_impl(t *testing.T) {
+	var _ statemgr.Reader = new(State)
+	var _ statemgr.Writer = new(State)
+	var _ statemgr.Persister = new(State)
+	var _ statemgr.Refresher = new(State)
+	var _ statemgr.OutputReader = new(State)
+	var _ statemgr.Locker = new(State)
+}
+
+type ExpectedOutput struct {
+	Name      string
+	Sensitive bool
+	IsNull    bool
+}
+
+func TestState_GetRootOutputValues(t *testing.T) {
+	b, bCleanup := testBackendWithOutputs(t)
+	defer bCleanup()
+
+	client := &remoteClient{
+		client: b.client,
+		workspace: &tfe.Workspace{
+			ID: "ws-abcd",
+		},
+	}
+
+	state := NewState(client)
+	outputs, err := state.GetRootOutputValues()
+
+	if err != nil {
+		t.Fatalf("error returned from GetRootOutputValues: %s", err)
+	}
+
+	cases := []ExpectedOutput{
+		{
+			Name:      "sensitive_output",
+			Sensitive: true,
+			IsNull:    false,
+		},
+		{
+			Name:      "nonsensitive_output",
+			Sensitive: false,
+			IsNull:    false,
+		},
+		{
+			Name:      "object_output",
+			Sensitive: false,
+			IsNull:    false,
+		},
+		{
+			Name:      "list_output",
+			Sensitive: false,
+			IsNull:    false,
+		},
+	}
+
+	if len(outputs) != len(cases) {
+		t.Errorf("Expected %d item but %d were returned", len(cases), len(outputs))
+	}
+
+	for _, testCase := range cases {
+		so, ok := outputs[testCase.Name]
+		if !ok {
+			t.Fatalf("Expected key %s but it was not found", testCase.Name)
+		}
+		if so.Value.IsNull() != testCase.IsNull {
+			t.Errorf("Key %s does not match null expectation %v", testCase.Name, testCase.IsNull)
+		}
+		if so.Sensitive != testCase.Sensitive {
+			t.Errorf("Key %s does not match sensitive expectation %v", testCase.Name, testCase.Sensitive)
+		}
+	}
+}

internal/cloud/testing.go

@@ -2,6 +2,7 @@ package cloud
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"io"
 	"net/http"
@@ -117,7 +118,69 @@ func testRemoteClient(t *testing.T) remote.Client {
 		t.Fatalf("error: %v", err)
 	}
 
-	return raw.(*remote.State).Client
+	return raw.(*State).Client
+}
+
+func testBackendWithOutputs(t *testing.T) (*Cloud, func()) {
+	b, cleanup := testBackendWithName(t)
+
+	// Get a new mock client to use for adding outputs
+	mc := NewMockClient()
+
+	mc.StateVersionOutputs.create("svo-abcd", &tfe.StateVersionOutput{
+		ID:           "svo-abcd",
+		Value:        "foobar",
+		Sensitive:    true,
+		Type:         "string",
+		Name:         "sensitive_output",
+		DetailedType: "string",
+	})
+
+	mc.StateVersionOutputs.create("svo-zyxw", &tfe.StateVersionOutput{
+		ID:           "svo-zyxw",
+		Value:        "bazqux",
+		Type:         "string",
+		Name:         "nonsensitive_output",
+		DetailedType: "string",
+	})
+
+	var dt interface{}
+	var val interface{}
+	err := json.Unmarshal([]byte(`["object", {"foo":"string"}]`), &dt)
+	if err != nil {
+		t.Fatalf("could not unmarshal detailed type: %s", err)
+	}
+	err = json.Unmarshal([]byte(`{"foo":"bar"}`), &val)
+	if err != nil {
+		t.Fatalf("could not unmarshal value: %s", err)
+	}
+	mc.StateVersionOutputs.create("svo-efgh", &tfe.StateVersionOutput{
+		ID:           "svo-efgh",
+		Value:        val,
+		Type:         "object",
+		Name:         "object_output",
+		DetailedType: dt,
+	})
+
+	err = json.Unmarshal([]byte(`["list", "bool"]`), &dt)
+	if err != nil {
+		t.Fatalf("could not unmarshal detailed type: %s", err)
+	}
+	err = json.Unmarshal([]byte(`[true, false, true, true]`), &val)
+	if err != nil {
+		t.Fatalf("could not unmarshal value: %s", err)
+	}
+	mc.StateVersionOutputs.create("svo-ijkl", &tfe.StateVersionOutput{
+		ID:           "svo-ijkl",
+		Value:        val,
+		Type:         "array",
+		Name:         "list_output",
+		DetailedType: dt,
+	})
+
+	b.client.StateVersionOutputs = mc.StateVersionOutputs
+
+	return b, cleanup
 }
 
 func testBackend(t *testing.T, obj cty.Value) (*Cloud, func()) {
@@ -149,6 +212,7 @@ func testBackend(t *testing.T, obj cty.Value) (*Cloud, func()) {
 	b.client.PolicyChecks = mc.PolicyChecks
 	b.client.Runs = mc.Runs
 	b.client.StateVersions = mc.StateVersions
+	b.client.StateVersionOutputs = mc.StateVersionOutputs
 	b.client.Variables = mc.Variables
 	b.client.Workspaces = mc.Workspaces
 

internal/cloud/tfe_client_mock.go

@@ -30,6 +30,7 @@ type MockClient struct {
 	PolicyChecks          *MockPolicyChecks
 	Runs                  *MockRuns
 	StateVersions         *MockStateVersions
+	StateVersionOutputs   *MockStateVersionOutputs
 	Variables             *MockVariables
 	Workspaces            *MockWorkspaces
 }
@@ -44,6 +45,7 @@ func NewMockClient() *MockClient {
 	c.PolicyChecks = newMockPolicyChecks(c)
 	c.Runs = newMockRuns(c)
 	c.StateVersions = newMockStateVersions(c)
+	c.StateVersionOutputs = newMockStateVersionOutputs(c)
 	c.Variables = newMockVariables(c)
 	c.Workspaces = newMockWorkspaces(c)
 	return c
@@ -1029,6 +1031,49 @@ func (m *MockStateVersions) ListOutputs(ctx context.Context, svID string, option
 	panic("not implemented")
 }
 
+type MockStateVersionOutputs struct {
+	client  *MockClient
+	outputs map[string]*tfe.StateVersionOutput
+}
+
+func newMockStateVersionOutputs(client *MockClient) *MockStateVersionOutputs {
+	return &MockStateVersionOutputs{
+		client:  client,
+		outputs: make(map[string]*tfe.StateVersionOutput),
+	}
+}
+
+// This is a helper function in order to create mocks to be read later
+func (m *MockStateVersionOutputs) create(id string, svo *tfe.StateVersionOutput) {
+	m.outputs[id] = svo
+}
+
+func (m *MockStateVersionOutputs) Read(ctx context.Context, outputID string) (*tfe.StateVersionOutput, error) {
+	result, ok := m.outputs[outputID]
+	if !ok {
+		return nil, tfe.ErrResourceNotFound
+	}
+
+	return result, nil
+}
+
+func (m *MockStateVersionOutputs) ReadCurrent(ctx context.Context, workspaceID string) (*tfe.StateVersionOutputsList, error) {
+	svl := &tfe.StateVersionOutputsList{}
+	for _, sv := range m.outputs {
+		svl.Items = append(svl.Items, sv)
+	}
+
+	svl.Pagination = &tfe.Pagination{
+		CurrentPage:  1,
+		NextPage:     1,
+		PreviousPage: 1,
+		TotalPages:   1,
+		TotalCount:   len(svl.Items),
+	}
+
+	return svl, nil
+}
+
 type MockVariables struct {
 	client     *MockClient
 	workspaces map[string]*tfe.VariableList

internal/command/output.go

@@ -82,17 +82,12 @@ func (c *OutputCommand) Outputs(statePath string) (map[string]*states.OutputValu
 		return nil, diags
 	}
 
-	if err := stateStore.RefreshState(); err != nil {
-		diags = diags.Append(fmt.Errorf("Failed to load state: %s", err))
-		return nil, diags
+	output, err := stateStore.GetRootOutputValues()
+	if err != nil {
+		return nil, diags.Append(err)
 	}
 
-	state := stateStore.State()
-	if state == nil {
-		state = states.NewState()
-	}
-
-	return state.RootModule().OutputValues, nil
+	return output, diags
 }
 
 func (c *OutputCommand) Help() string {

internal/states/remote/state.go

@@ -46,6 +46,19 @@ func (s *State) State() *states.State {
 	return s.state.DeepCopy()
 }
 
+func (s *State) GetRootOutputValues() (map[string]*states.OutputValue, error) {
+	if err := s.RefreshState(); err != nil {
+		return nil, fmt.Errorf("Failed to load state: %s", err)
+	}
+
+	state := s.State()
+	if state == nil {
+		state = states.NewState()
+	}
+
+	return state.RootModule().OutputValues, nil
+}
+
 // StateForMigration is part of our implementation of statemgr.Migrator.
 func (s *State) StateForMigration() *statefile.File {
 	s.mu.Lock()

internal/states/remote/state_test.go

@@ -19,6 +19,7 @@ func TestState_impl(t *testing.T) {
 	var _ statemgr.Writer = new(State)
 	var _ statemgr.Persister = new(State)
 	var _ statemgr.Refresher = new(State)
+	var _ statemgr.OutputReader = new(State)
 	var _ statemgr.Locker = new(State)
 }
 
@@ -276,6 +277,33 @@ func TestStatePersist(t *testing.T) {
 	}
 }
 
+func TestState_GetRootOutputValues(t *testing.T) {
+	// Initial setup of state with outputs already defined
+	mgr := &State{
+		Client: &mockClient{
+			current: []byte(`
+				{
+					"version": 4,
+					"lineage": "mock-lineage",
+					"serial": 1,
+					"terraform_version":"0.0.0",
+					"outputs": {"foo": {"value":"bar", "type": "string"}},
+					"resources": []
+				}
+			`),
+		},
+	}
+
+	outputs, err := mgr.GetRootOutputValues()
+	if err != nil {
+		t.Errorf("Expected GetRootOutputValues to not return an error, but it returned %v", err)
+	}
+
+	if len(outputs) != 1 {
+		t.Errorf("Expected %d outputs, but received %d", 1, len(outputs))
+	}
+}
+
 type migrationTestCase struct {
 	name string
 	// A function to generate a statefile

internal/states/statemgr/filesystem.go

@@ -233,6 +233,20 @@ func (s *Filesystem) RefreshState() error {
 	return s.refreshState()
 }
 
+func (s *Filesystem) GetRootOutputValues() (map[string]*states.OutputValue, error) {
+	err := s.RefreshState()
+	if err != nil {
+		return nil, err
+	}
+
+	state := s.State()
+	if state == nil {
+		state = states.NewState()
+	}
+
+	return state.RootModule().OutputValues, nil
+}
+
 func (s *Filesystem) refreshState() error {
 	var reader io.Reader
 

internal/states/statemgr/filesystem_test.go

@@ -336,6 +336,7 @@ func TestFilesystem_impl(t *testing.T) {
 	var _ Writer = new(Filesystem)
 	var _ Persister = new(Filesystem)
 	var _ Refresher = new(Filesystem)
+	var _ OutputReader = new(Filesystem)
 	var _ Locker = new(Filesystem)
 }
 
@@ -410,6 +411,19 @@ func TestFilesystem_refreshWhileLocked(t *testing.T) {
 	}
 }
 
+func TestFilesystem_GetRootOutputValues(t *testing.T) {
+	fs := testFilesystem(t)
+
+	outputs, err := fs.GetRootOutputValues()
+	if err != nil {
+		t.Errorf("Expected GetRootOutputValues to not return an error, but it returned %v", err)
+	}
+
+	if len(outputs) != 2 {
+		t.Errorf("Expected %d outputs, but received %d", 2, len(outputs))
+	}
+}
+
 func testOverrideVersion(t *testing.T, v string) func() {
 	oldVersionStr := tfversion.Version
 	oldPrereleaseStr := tfversion.Prerelease

internal/states/statemgr/lock.go

@@ -15,6 +15,10 @@ func (s *LockDisabled) State() *states.State {
 	return s.Inner.State()
 }
 
+func (s *LockDisabled) GetRootOutputValues() (map[string]*states.OutputValue, error) {
+	return s.Inner.GetRootOutputValues()
+}
+
 func (s *LockDisabled) WriteState(v *states.State) error {
 	return s.Inner.WriteState(v)
 }

internal/states/statemgr/persistent.go

@@ -2,6 +2,7 @@ package statemgr
 
 import (
 	version "github.com/hashicorp/go-version"
+	"github.com/hashicorp/terraform/internal/states"
 )
 
 // Persistent is a union of the Refresher and Persistent interfaces, for types
@@ -16,6 +17,16 @@ import (
 type Persistent interface {
 	Refresher
 	Persister
+	OutputReader
+}
+
+// OutputReader is the interface for managers that fetches output values from state
+// or another source. This is a refinement of fetching the entire state and digging
+// the output values from it because enhanced backends can apply special permissions
+// to differentiate reading the state and reading the outputs within the state.
+type OutputReader interface {
+	// GetRootOutputValues fetches the root module output values from state or another source
+	GetRootOutputValues() (map[string]*states.OutputValue, error)
 }
 
 // Refresher is the interface for managers that can read snapshots from

internal/states/statemgr/statemgr_fake.go

@@ -65,6 +65,10 @@ func (m *fakeFull) PersistState() error {
 	return m.fakeP.WriteState(m.t.State())
 }
 
+func (m *fakeFull) GetRootOutputValues() (map[string]*states.OutputValue, error) {
+	return m.State().RootModule().OutputValues, nil
+}
+
 func (m *fakeFull) Lock(info *LockInfo) (string, error) {
 	m.lockLock.Lock()
 	defer m.lockLock.Unlock()
@@ -111,6 +115,10 @@ func (m *fakeErrorFull) State() *states.State {
 	return nil
 }
 
+func (m *fakeErrorFull) GetRootOutputValues() (map[string]*states.OutputValue, error) {
+	return nil, errors.New("fake state manager error")
+}
+
 func (m *fakeErrorFull) WriteState(s *states.State) error {
 	return errors.New("fake state manager error")
 }

internal/states/statemgr/testing.go

@@ -155,5 +155,9 @@ func TestFullInitialState() *states.State {
 		Module:   addrs.RootModule,
 	}
 	childMod.SetResourceProvider(rAddr, providerAddr)
+
+	state.RootModule().SetOutputValue("sensitive_output", cty.StringVal("it's a secret"), true)
+	state.RootModule().SetOutputValue("nonsensitive_output", cty.StringVal("hello, world!"), false)
+
 	return state
 }