Skip to content

Commit

Permalink
Improve Retention Policy API / UI / CLI
Browse files Browse the repository at this point in the history 
- Proper HTTP Verb (PATCH not PUT)
- Better bounds checking for expiry days and policy name length.
- Update API docs to match

Fixes https://trello.com/c/Er2n1hnP
Fixes https://trello.com/c/iLk5wUSO
Fixes https://trello.com/c/7iCoEiWG
  • Loading branch information
@dennisjbell @jhunt
dennisjbell authored and jhunt committed Jul 26, 2018
1 parent 7a69639 commit 21f1ac8
Show file tree
Hide file tree
Showing 9 changed files with 176 additions and 65 deletions.
2 changes: 1 addition & 1 deletion client/v2/shield/policies.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ func (c *Client) CreatePolicyTemplate(in *Policy) (*Policy, error) {
func (c *Client) UpdatePolicyTemplate(in *Policy) (*Policy, error) {
fixupPolicyRequest(in)
var out *Policy
if err := c.put(fmt.Sprintf("/v2/global/policies/%s", in.UUID), in, &out); err != nil {
if err := c.patch(fmt.Sprintf("/v2/global/policies/%s", in.UUID), in, &out); err != nil {
return nil, err
}
fixupPolicyResponse(out)
Expand Down
4 changes: 0 additions & 4 deletions cmd/shield/help.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -349,8 +349,6 @@ func ShowHelp(command string) {
fmt.Printf(" -n, --name The name of your new Retention Policy.\n")
fmt.Printf(" This field is @W{required}.\n")
fmt.Printf("\n")
fmt.Printf(" -s, --summary An optional, long-form description for the policy.\n")
fmt.Printf("\n")
fmt.Printf(" -d, --days How many days to keep backup archives for.\n")
fmt.Printf(" This field is @W{required}, and must be a positive,\n")
fmt.Printf(" non-zero, whole number.\n")
Expand Down Expand Up @@ -1672,8 +1670,6 @@ func ShowHelp(command string) {
fmt.Printf("\n")
fmt.Printf(" -n, --name A new name for the Retention Policy.\n")
fmt.Printf("\n")
fmt.Printf(" -s, --summary An optional, long-form description for the policy.\n")
fmt.Printf("\n")
fmt.Printf(" -d, --days How many days to keep backup archives for.\n")
fmt.Printf(" This field is @W{required}, and must be a positive,\n")
fmt.Printf(" non-zero, whole number.\n")
Expand Down
88 changes: 71 additions & 17 deletions cmd/shield/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1896,19 +1896,37 @@ func main() {

if !opts.Batch {
if opts.CreatePolicy.Name == "" {
opts.CreatePolicy.Name = prompt("@C{Policy Name}: ")
for {
n := prompt("@C{Policy Name}: ")
if len(n) == 0 {
fmt.Fprintf(os.Stderr, "@R{invalid name (must not be blank)}\n")
continue
}
if len(n) > 100 {
fmt.Fprintf(os.Stderr, "@R{invalid name (must not be more than 100 characters)}\n")
continue
}
opts.CreatePolicy.Name = n
break
}
}
if opts.CreatePolicy.Summary == "" {
opts.CreatePolicy.Summary = prompt("@C{Summary}: ")
}
if opts.CreatePolicy.Days == 0 {
for {
s := prompt("@C{Retention Period (days)}: ")
if d, err := strconv.Atoi(s); err == nil && d > 0 {
opts.CreatePolicy.Days = d
break
d, err := strconv.Atoi(s)
if err != nil || d == 0 {
fmt.Fprintf(os.Stderr, "@R{invalid expiry (must be numeric and greater than zero)}\n")
continue
}
if d > 366*10 {
fmt.Fprintf(os.Stderr, "@R{invalid expiry (must not exceed 3660 days (~10 years))}\n")
continue
}
fmt.Fprintf(os.Stderr, "@R{invalid expiry (must be numeric and greater than zero)}\n")
opts.CreatePolicy.Days = d
break
}
}
}
Expand Down Expand Up @@ -1944,12 +1962,21 @@ func main() {
bail(err)

if opts.UpdatePolicy.Name != "" {
if len(opts.UpdatePolicy.Name) > 100 {
fail(2, "@R{Policy name cannot exceed 100 characters.}\n\n")
}
p.Name = opts.UpdatePolicy.Name
}
if opts.UpdatePolicy.Summary != "" {
p.Summary = opts.UpdatePolicy.Summary
}
if opts.UpdatePolicy.Days != 0 {
if opts.UpdatePolicy.Days < 1 {
fail(2, "@R{Policy expiry must be a positive non-zero integer}\n\n")
}
if opts.UpdatePolicy.Days > 366*10 {
fail(2, "@R{Policy expiry must be no more than 3660 days (~10 years)}\n\n")
}
p.Expires = opts.UpdatePolicy.Days
}

Expand All @@ -1964,7 +1991,7 @@ func main() {
r := tui.NewReport()
r.Add("UUID", p.UUID)
r.Add("Name", p.Name)
r.Add("Retention Period", fmt.Sprintf("%dd", p.Expires))
r.Add("Retention Period", fmt.Sprintf("%dd", p.Expires/86400))
r.Output(os.Stdout)

/* }}} */
Expand Down Expand Up @@ -2025,23 +2052,41 @@ func main() {
r := tui.NewReport()
r.Add("UUID", p.UUID)
r.Add("Name", p.Name)
r.Add("Retention Period", fmt.Sprintf("%dd", p.Expires))
r.Add("Retention Period", fmt.Sprintf("%dd", p.Expires/86400))
r.Output(os.Stdout)

/* }}} */
case "create-policy-template": /* {{{ */
if !opts.Batch {
if opts.CreatePolicyTemplate.Name == "" {
opts.CreatePolicyTemplate.Name = prompt("@C{Policy Template Name}: ")
for {
n := prompt("@C{Policy Template Name}: ")
if len(n) == 0 {
fmt.Fprintf(os.Stderr, "@R{invalid name (must not be blank)}\n")
continue
}
if len(n) > 100 {
fmt.Fprintf(os.Stderr, "@R{invalid name (must not be more than 100 characters)}\n")
continue
}
opts.CreatePolicyTemplate.Name = n
break
}
}
if opts.CreatePolicyTemplate.Days == 0 {
for {
s := prompt("@C{Retention Period (days)}: ")
if d, err := strconv.Atoi(s); err != nil && d > 0 {
opts.CreatePolicyTemplate.Days = d
break
d, err := strconv.Atoi(s)
if err != nil || d == 0 {
fmt.Fprintf(os.Stderr, "@R{invalid expiry (must be numeric and greater than zero)}\n")
continue
}
if d > 366*10 {
fmt.Fprintf(os.Stderr, "@R{invalid expiry (must not exceed 3660 days (~10 years))}\n")
continue
}
fmt.Fprintf(os.Stderr, "@R{invalid expiry (must be numeric and greater than zero)}\n")
opts.CreatePolicyTemplate.Days = d
break
}
}
}
Expand Down Expand Up @@ -2071,11 +2116,20 @@ func main() {
p, err := c.FindPolicyTemplate(args[0], true)
bail(err)

if opts.UpdatePolicy.Name != "" {
p.Name = opts.UpdatePolicy.Name
if opts.UpdatePolicyTemplate.Name != "" {
if len(opts.UpdatePolicyTemplate.Name) > 100 {
fail(2, "@R{Policy name cannot exceed 100 characters.}\n\n")
}
p.Name = opts.UpdatePolicyTemplate.Name
}
if opts.UpdatePolicy.Days != 0 {
p.Expires = opts.UpdatePolicy.Days
if opts.UpdatePolicyTemplate.Days != 0 {
if opts.UpdatePolicyTemplate.Days < 1 {
fail(2, "@R{Policy expiry must be a positive non-zero integer}\n\n")
}
if opts.UpdatePolicyTemplate.Days > 366*10 {
fail(2, "@R{Policy expiry must be no more than 3660 days (~10 years)}\n\n")
}
p.Expires = opts.UpdatePolicyTemplate.Days
}

_, err = c.UpdatePolicyTemplate(p)
Expand All @@ -2089,7 +2143,7 @@ func main() {
r := tui.NewReport()
r.Add("UUID", p.UUID)
r.Add("Name", p.Name)
r.Add("Retention Period", fmt.Sprintf("%dd", p.Expires))
r.Add("Retention Period", fmt.Sprintf("%dd", p.Expires/86400))
r.Output(os.Stdout)

/* }}} */
Expand Down
36 changes: 35 additions & 1 deletion core/v2.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1753,11 +1753,21 @@ func (core *Core) v2API() *route.Router {
return
}

if len(in.Name) > 100 {
r.Fail(route.Bad(nil, "Retention policy name must not exceed 100 characters"))
return
}

/* FIXME: for v2, flip expires over to days, not seconds */
if in.Expires < 86400 {
r.Fail(route.Bad(nil, "Retention policy expiry must be greater than 1 day"))
return
}
if in.Expires > 10*366*86400 {
r.Fail(route.Bad(nil, "Retention policy expiry must not exceed than 3660 days (~10 years)"))
return
}

if in.Expires%86400 != 0 {
r.Fail(route.Bad(nil, "Retention policy expiry must be a multiple of 1 day"))
return
Expand Down Expand Up @@ -1827,6 +1837,10 @@ func (core *Core) v2API() *route.Router {
}

if in.Name != "" {
if len(in.Name) > 100 {
r.Fail(route.Bad(nil, "Retention policy name must not exceed 100 characters"))
return
}
policy.Name = in.Name
}
if in.Summary != "" {
Expand All @@ -1838,6 +1852,10 @@ func (core *Core) v2API() *route.Router {
r.Fail(route.Bad(nil, "Retention policy expiry must be greater than 1 day"))
return
}
if in.Expires > 10*366*86400 {
r.Fail(route.Bad(nil, "Retention policy expiry must not exceed than 3660 days (~10 years)"))
return
}
if in.Expires%86400 != 0 {
r.Fail(route.Bad(nil, "Retention policy expiry must be a multiple of 1 day"))
return
Expand Down Expand Up @@ -3074,12 +3092,20 @@ func (core *Core) v2API() *route.Router {
if r.Missing("name", in.Name) {
return
}
if len(in.Name) > 100 {
r.Fail(route.Bad(nil, "Retention policy name must not exceed 100 characters"))
return
}

/* FIXME: for v2, flip expires over to days, not seconds */
if in.Expires < 86400 {
r.Fail(route.Bad(nil, "Retention policy expiry must be greater than 1 day"))
return
}
if in.Expires > 10*366*86400 {
r.Fail(route.Bad(nil, "Retention policy expiry must not exceed than 3660 days (~10 years)"))
return
}
if in.Expires%86400 != 0 {
r.Fail(route.Bad(nil, "Retention policy expiry must be a multiple of 1 day"))
return
Expand Down Expand Up @@ -3118,7 +3144,7 @@ func (core *Core) v2API() *route.Router {
r.OK(policy)
})
// }}}
r.Dispatch("PUT /v2/global/policies/:uuid", func(r *route.Request) { // {{{
r.Dispatch("PATCH /v2/global/policies/:uuid", func(r *route.Request) { // {{{
if core.IsNotSystemEngineer(r) {
return
}
Expand All @@ -3144,6 +3170,10 @@ func (core *Core) v2API() *route.Router {
}

if in.Name != "" {
if len(in.Name) > 100 {
r.Fail(route.Bad(nil, "Retention policy name must not exceed 100 characters"))
return
}
policy.Name = in.Name
}
if in.Summary != "" {
Expand All @@ -3155,6 +3185,10 @@ func (core *Core) v2API() *route.Router {
r.Fail(route.Bad(nil, "Retention policy expiry must be greater than 1 day"))
return
}
if in.Expires > 10*366*86400 {
r.Fail(route.Bad(nil, "Retention policy expiry must not exceed than 3660 days (~10 years)"))
return
}
if in.Expires%86400 != 0 {
r.Fail(route.Bad(nil, "Retention policy expiry must be a multiple of 1 day"))
return
Expand Down
4 changes: 2 additions & 2 deletions docs/API.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5380,7 +5380,7 @@ The following error messages can be returned:
after authentication.


### PUT /v2/global/policies/:uuid
### PATCH /v2/global/policies/:uuid

Update a single retention policy template.

Expand All @@ -5389,7 +5389,7 @@ Update a single retention policy template.

curl -H 'Accept: application/json' \
-H 'Content-Type: application/json' \
-X PUT https://shield.host/v2/global/policies/:uuid \
-X PATCH https://shield.host/v2/global/policies/:uuid \
--data-binary '
{
"name" : "Updated Retention Policy Name",
Expand Down
2 changes: 1 addition & 1 deletion docs/API.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3904,7 +3904,7 @@ sections:
# }}}
- name: PUT /v2/global/policies/:uuid # {{{
- name: PATCH /v2/global/policies/:uuid # {{{
intro: |
Update a single retention policy template.
access: [system, engineer]
Expand Down
11 changes: 8 additions & 3 deletions web2/htdocs/index.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1177,9 +1177,10 @@ <h3>Configure a New Retention Policy</h3>[[# {{{ ]]
<label for="policy-name">Name</label>
<span class="errors">
<span data-error="missing">You must provide a name for this new retention policy.</span>
<span data-error="too-big">The name cannot exceed 100 characters</span>
</span>
<div class="widget">
<input type="text" id="policy-name" name="name" class="autofocus" tabindex="0" />
<input type="text" id="policy-name" name="name" maxlength="100" class="autofocus" tabindex="0" />
<p class="help">Pick a memorable or descriptive name for this retention policy,
so that other operators can determine what to use it for.</p>
</div>
Expand All @@ -1190,6 +1191,7 @@ <h3>Configure a New Retention Policy</h3>[[# {{{ ]]
<span class="errors">
<span data-error="missing">You must specify an expiry of at least 1 day.</span>
<span data-error="invalid">This must be a positive, non-zero, whole number.</span>
<span data-error="too-big">Retention expiry cannot exceed 10 years (3660 days)</span>
</span>
<div class="widget">
<input type="text" id="policy-days" name="days" />
Expand Down Expand Up @@ -1962,10 +1964,11 @@ <h2>[[ if (_.policy) { ]]Edit[[ } else { ]]New[[ } ]] Retention Policy[[= _.admi
<div class="ctl" data-field="name">
<label for="policy-name">Name</label>
<span class="errors">
<span data-error="missing">You must provide a name for this new retention policy.</span>
<span data-error="missing">You must provide a name for this [[= (_.policy) ? "existing" : "new" ]] retention policy.</span>
<span data-error="too-big">The name cannot exceed 100 characters</span>
</span>
<div class="widget">
<input type="text" id="policy-name" name="name" class="autofocus" tabindex="0"
<input type="text" id="policy-name" name="name" maxlength="100" class="autofocus" tabindex="0"
[[ if (_.policy) { ]]value="[[= _.policy.name ]]"[[ } ]]/>
<p class="help">Pick a memorable or descriptive name for this retention policy,
so that other operators can determine what to use it for.</p>
Expand All @@ -1977,6 +1980,8 @@ <h2>[[ if (_.policy) { ]]Edit[[ } else { ]]New[[ } ]] Retention Policy[[= _.admi
<span class="errors">
<span data-error="missing">You must specify an expiry of at least 1 day.</span>
<span data-error="invalid">This must be a positive, non-zero, whole number.</span>
<span data-error="too-big">Retention expiry cannot exceed 10 years (3660 days)</span>

</span>
<div class="widget">
<input type="text" id="policy-days" name="days"
Expand Down
Loading

0 comments on commit 21f1ac8

Please sign in to comment.