Skip to content

Commit b19ac45

Browse files
leg100leg100
authored
Fix update of sensitive variables (#345)
1 parent 4173006 commit b19ac45

File tree

6 files changed

+181
-36
lines changed

6 files changed

+181
-36
lines changed

‎e2e/variables_test.go

+13
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,19 @@ output "foo" {
131131
screenshot(t),
132132
// confirm value is hidden (because it is sensitive)
133133
matchText(t, `//table[@class='variables']/tbody/tr/td[2]`, "hidden"),
134+
// edit variable again
135+
chromedp.Click(`//a[text()='foo']`, chromedp.NodeVisible),
136+
screenshot(t),
137+
// update value
138+
chromedp.Focus("textarea#value", chromedp.NodeVisible),
139+
input.InsertText("topsecret"),
140+
screenshot(t),
141+
// submit form
142+
chromedp.Click(`//button[text()='Save variable']`, chromedp.NodeVisible),
143+
screenshot(t),
144+
// confirm variable updated
145+
matchText(t, ".flash-success", "updated variable: foo"),
146+
screenshot(t),
134147
// delete variable
135148
chromedp.Click(`//button[text()='Delete']`, chromedp.NodeVisible),
136149
screenshot(t),

‎variable/test_helpers.go

+4
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,10 @@ type fakeService struct {
1919
Service
2020
}
2121

22+
func (f *fakeService) GetVariable(ctx context.Context, variableID string) (*Variable, error) {
23+
return f.variable, nil
24+
}
25+
2226
func (f *fakeService) UpdateVariable(ctx context.Context, variableID string, opts UpdateVariableOptions) (*Variable, error) {
2327
if err := f.variable.Update(opts); err != nil {
2428
return nil, err

‎variable/variable.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ func (v *Variable) Update(opts UpdateVariableOptions) error {
154154
}
155155
if opts.HCL != nil {
156156
if v.Sensitive {
157-
return errors.New("toggling HCL mode on a sensitive variable is not allowed")
157+
return errors.New("changing HCL mode on a sensitive variable is not allowed")
158158
}
159159
v.HCL = *opts.HCL
160160
}

‎variable/variable_test.go

+105
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,111 @@ import (
1010
"github.com/stretchr/testify/require"
1111
)
1212

13+
func TestUpdateVariable(t *testing.T) {
14+
tests := []struct {
15+
name string
16+
opts UpdateVariableOptions
17+
before Variable
18+
after Variable
19+
err bool // want error
20+
}{
21+
{
22+
name: "no change",
23+
opts: UpdateVariableOptions{},
24+
before: Variable{
25+
Key: "foo",
26+
Value: "bar",
27+
Category: CategoryTerraform,
28+
},
29+
after: Variable{
30+
Key: "foo",
31+
Value: "bar",
32+
Category: CategoryTerraform,
33+
},
34+
},
35+
{
36+
name: "key",
37+
opts: UpdateVariableOptions{Key: otf.String("teddy")},
38+
before: Variable{
39+
Key: "foo",
40+
Value: "bar",
41+
Category: CategoryTerraform,
42+
},
43+
after: Variable{
44+
Key: "teddy",
45+
Value: "bar",
46+
Category: CategoryTerraform,
47+
},
48+
},
49+
{
50+
name: "value",
51+
opts: UpdateVariableOptions{Value: otf.String("baz")},
52+
before: Variable{
53+
Key: "foo",
54+
Value: "bar",
55+
Category: CategoryTerraform,
56+
},
57+
after: Variable{
58+
Key: "foo",
59+
Value: "baz",
60+
Category: CategoryTerraform,
61+
},
62+
},
63+
{
64+
name: "non-sensitive to sensitive",
65+
opts: UpdateVariableOptions{Sensitive: otf.Bool(true)},
66+
before: Variable{
67+
Key: "foo",
68+
Value: "bar",
69+
Category: CategoryTerraform,
70+
},
71+
after: Variable{
72+
Key: "foo",
73+
Value: "bar",
74+
Category: CategoryTerraform,
75+
Sensitive: true,
76+
},
77+
},
78+
{
79+
name: "non-hcl to hcl",
80+
opts: UpdateVariableOptions{HCL: otf.Bool(true)},
81+
before: Variable{
82+
Key: "foo",
83+
Value: "bar",
84+
Category: CategoryTerraform,
85+
},
86+
after: Variable{
87+
Key: "foo",
88+
Value: "bar",
89+
Category: CategoryTerraform,
90+
HCL: true,
91+
},
92+
},
93+
{
94+
name: "sensitive to non-sensitive",
95+
opts: UpdateVariableOptions{Sensitive: otf.Bool(false)},
96+
before: Variable{
97+
Key: "foo",
98+
Value: "bar",
99+
Category: CategoryTerraform,
100+
Sensitive: true,
101+
},
102+
err: true,
103+
},
104+
}
105+
for _, tt := range tests {
106+
t.Run(tt.name, func(t *testing.T) {
107+
got := tt.before
108+
err := got.Update(tt.opts)
109+
if tt.err {
110+
assert.Error(t, err)
111+
} else {
112+
assert.Equal(t, tt.after, got)
113+
}
114+
})
115+
}
116+
}
117+
13118
func TestWriteTerraformVariables(t *testing.T) {
14119
dir := t.TempDir()
15120

‎variable/web.go

+46-20
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ func (h *web) new(w http.ResponseWriter, r *http.Request) {
5656
}
5757

5858
func (h *web) create(w http.ResponseWriter, r *http.Request) {
59-
type parameters struct {
59+
var params struct {
6060
Key *string `schema:"key,required"`
6161
Value *string
6262
Description *string
@@ -65,7 +65,6 @@ func (h *web) create(w http.ResponseWriter, r *http.Request) {
6565
HCL bool
6666
WorkspaceID string `schema:"workspace_id,required"`
6767
}
68-
var params parameters
6968
if err := decode.All(&params, r); err != nil {
7069
html.Error(w, err.Error(), http.StatusUnprocessableEntity)
7170
return
@@ -147,36 +146,63 @@ func (h *web) edit(w http.ResponseWriter, r *http.Request) {
147146
}
148147

149148
func (h *web) update(w http.ResponseWriter, r *http.Request) {
150-
type parameters struct {
151-
Key *string `schema:"key,required"`
149+
variableID, err := decode.Param("variable_id", r)
150+
if err != nil {
151+
html.Error(w, err.Error(), http.StatusUnprocessableEntity)
152+
return
153+
}
154+
155+
variable, err := h.svc.GetVariable(r.Context(), variableID)
156+
if err != nil {
157+
html.Error(w, err.Error(), http.StatusInternalServerError)
158+
return
159+
}
160+
161+
// Handle updates to sensitive variables in a separate handler.
162+
if variable.Sensitive {
163+
h.updateSensitive(w, r, variable)
164+
return
165+
}
166+
167+
var params struct {
168+
Key *string
152169
Value *string
153170
Description *string
154-
Category *VariableCategory `schema:"category,required"`
155-
Sensitive bool
156-
HCL bool
157-
VariableID string `schema:"variable_id,required"`
171+
Category *VariableCategory
172+
Sensitive *bool // form checkbox can only be true/false, not nil
173+
HCL *bool // form checkbox can only be true/false, not nil
158174
}
159-
var params parameters
160175
if err := decode.All(&params, r); err != nil {
161176
html.Error(w, err.Error(), http.StatusUnprocessableEntity)
162177
return
163178
}
164179

165-
// sensitive variable's value form field is deliberately empty, so avoid
166-
// updating value with empty string (this does mean users cannot set
167-
// a sensitive variable's value to an empty string but there unlikely to be
168-
// a valid reason to want to do that...)
169-
if params.Sensitive && params.Value != nil && *params.Value == "" {
170-
params.Value = nil
171-
}
172-
173-
variable, err := h.svc.UpdateVariable(r.Context(), params.VariableID, UpdateVariableOptions{
180+
variable, err = h.svc.UpdateVariable(r.Context(), variableID, UpdateVariableOptions{
174181
Key: params.Key,
175182
Value: params.Value,
176183
Description: params.Description,
177184
Category: params.Category,
178-
Sensitive: &params.Sensitive,
179-
HCL: &params.HCL,
185+
Sensitive: params.Sensitive,
186+
HCL: params.HCL,
187+
})
188+
if err != nil {
189+
html.Error(w, err.Error(), http.StatusInternalServerError)
190+
return
191+
}
192+
193+
html.FlashSuccess(w, "updated variable: "+variable.Key)
194+
http.Redirect(w, r, paths.Variables(variable.WorkspaceID), http.StatusFound)
195+
}
196+
197+
func (h *web) updateSensitive(w http.ResponseWriter, r *http.Request, variable *Variable) {
198+
value, err := decode.Param("value", r)
199+
if err != nil {
200+
html.Error(w, err.Error(), http.StatusUnprocessableEntity)
201+
return
202+
}
203+
204+
variable, err = h.svc.UpdateVariable(r.Context(), variable.ID, UpdateVariableOptions{
205+
Value: otf.String(value),
180206
})
181207
if err != nil {
182208
html.Error(w, err.Error(), http.StatusInternalServerError)

‎variable/web_test.go

+12-15
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ func TestVariable_Update(t *testing.T) {
1717
tests := []struct {
1818
name string
1919
existing CreateVariableOptions
20-
form url.Values
20+
updated url.Values
2121
want func(t *testing.T, got *Variable)
2222
}{
2323
{
@@ -27,7 +27,7 @@ func TestVariable_Update(t *testing.T) {
2727
Value: otf.String("bar"),
2828
Category: VariableCategoryPtr(CategoryTerraform),
2929
},
30-
form: url.Values{
30+
updated: url.Values{
3131
"key": {"new-key"},
3232
"value": {"new-value"},
3333
"category": {"env"},
@@ -43,21 +43,18 @@ func TestVariable_Update(t *testing.T) {
4343
},
4444
},
4545
{
46-
name: "skip sensitive variable empty value",
46+
name: "update sensitive value",
4747
existing: CreateVariableOptions{
48-
Key: otf.String("foo"),
49-
Value: otf.String("bar"),
50-
Category: VariableCategoryPtr(CategoryTerraform),
48+
Key: otf.String("foo"),
49+
Value: otf.String("topsecret"),
50+
Category: VariableCategoryPtr(CategoryTerraform),
51+
Sensitive: otf.Bool(true),
5152
},
52-
form: url.Values{
53-
"key": {"foo"},
54-
"value": {""},
55-
"sensitive": {"on"},
56-
"category": {"terraform"},
53+
updated: url.Values{
54+
"value": {"evenmoretopsecret"},
5755
},
5856
want: func(t *testing.T, got *Variable) {
59-
// should get original value
60-
assert.Equal(t, "bar", got.Value)
57+
assert.Equal(t, "evenmoretopsecret", got.Value)
6158
},
6259
},
6360
}
@@ -66,13 +63,13 @@ func TestVariable_Update(t *testing.T) {
6663
// create existing variable for test to update
6764
v := NewTestVariable(t, "ws-123", tt.existing)
6865

69-
r := httptest.NewRequest("POST", "/?variable_id="+v.ID, strings.NewReader(tt.form.Encode()))
66+
r := httptest.NewRequest("POST", "/?variable_id="+v.ID, strings.NewReader(tt.updated.Encode()))
7067
r.Header.Add("Content-Type", "application/x-www-form-urlencoded")
7168
w := httptest.NewRecorder()
7269

7370
fakeHTMLApp(t, v).update(w, r)
7471

75-
if assert.Equal(t, 302, w.Code) {
72+
if assert.Equal(t, 302, w.Code, "got body: %s", w.Body.String()) {
7673
redirect, err := w.Result().Location()
7774
require.NoError(t, err)
7875
assert.Equal(t, paths.Variables(v.WorkspaceID), redirect.Path)

0 commit comments

Comments
 (0)