Skip to content

Commit

Permalink
addressed comments from review
Browse files Browse the repository at this point in the history
  • Loading branch information
chipaca committed Jul 14, 2016
1 parent 0ae5716 commit 5a1a52f
Show file tree
Hide file tree
Showing 13 changed files with 94 additions and 97 deletions.
6 changes: 3 additions & 3 deletions client/snap_op.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import (
type SnapOptions struct {
Channel string `json:"channel,omitempty"`
DevMode bool `json:"devmode,omitempty"`
Confined bool `json:"confined,omitempty"`
JailMode bool `json:"jailmode,omitempty"`
}

type actionData struct {
Expand Down Expand Up @@ -123,7 +123,7 @@ func (client *Client) Try(path string, options *SnapOptions) (changeID string, e
mw.WriteField("action", "try")
mw.WriteField("snap-path", path)
mw.WriteField("devmode", strconv.FormatBool(options.DevMode))
mw.WriteField("confined", strconv.FormatBool(options.Confined))
mw.WriteField("jailmode", strconv.FormatBool(options.JailMode))
mw.Close()

headers := map[string]string{
Expand All @@ -145,7 +145,7 @@ func sendSnapFile(snapPath string, snapFile *os.File, pw *io.PipeWriter, mw *mul
mw.WriteField("snap-path", action.SnapPath),
mw.WriteField("channel", action.Channel),
mw.WriteField("devmode", strconv.FormatBool(action.DevMode)),
mw.WriteField("confined", strconv.FormatBool(action.Confined)),
mw.WriteField("jailmode", strconv.FormatBool(action.JailMode)),
}
for _, err := range errs {
if err != nil {
Expand Down
10 changes: 5 additions & 5 deletions client/snap_op_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,10 +172,10 @@ func (cs *clientSuite) TestClientOpTryMode(c *check.C) {
snapdir := filepath.Join(c.MkDir(), "/some/path")

for _, opts := range []*client.SnapOptions{
{DevMode: false, Confined: false},
{DevMode: false, Confined: true},
{DevMode: true, Confined: true},
{DevMode: true, Confined: false},
{DevMode: false, JailMode: false},
{DevMode: false, JailMode: true},
{DevMode: true, JailMode: true},
{DevMode: true, JailMode: false},
} {
id, err := cs.cli.Try(snapdir, opts)
c.Assert(err, check.IsNil)
Expand All @@ -189,7 +189,7 @@ func (cs *clientSuite) TestClientOpTryMode(c *check.C) {
"action": "try",
"snap-path": snapdir,
"devmode": strconv.FormatBool(opts.DevMode),
"confined": strconv.FormatBool(opts.Confined),
"jailmode": strconv.FormatBool(opts.JailMode),
})

c.Check(cs.req.Method, check.Equals, "POST")
Expand Down
2 changes: 1 addition & 1 deletion cmd/snap/cmd_list_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,6 @@ func (s *SnapSuite) TestListWithNotes(c *check.C) {
c.Check(s.Stdout(), check.Matches, `(?ms).*^foo +4.2 +17 +bar +try$`)
c.Check(s.Stdout(), check.Matches, `(?ms).*^dm1 +.* +devmode$`)
c.Check(s.Stdout(), check.Matches, `(?ms).*^dm2 +.* +devmode$`)
c.Check(s.Stdout(), check.Matches, `(?ms).*^cf1 +.* +confined$`)
c.Check(s.Stdout(), check.Matches, `(?ms).*^cf1 +.* +jailmode$`)
c.Check(s.Stderr(), check.Equals, "")
}
4 changes: 2 additions & 2 deletions cmd/snap/cmd_snap_op.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ type cmdInstall struct {
channelMixin

DevMode bool `long:"devmode" description:"Install the snap with non-enforcing security"`
Confined bool `long:"confined" description:"Override a snap's request for non-enforcing security"`
JailMode bool `long:"jailmode" description:"Override a snap's request for non-enforcing security"`
Positional struct {
Snap string `positional-arg-name:"<snap>"`
} `positional-args:"yes" required:"yes"`
Expand All @@ -232,7 +232,7 @@ func (x *cmdInstall) Execute([]string) error {

cli := Client()
name := x.Positional.Snap
opts := &client.SnapOptions{Channel: x.Channel, DevMode: x.DevMode, Confined: x.Confined}
opts := &client.SnapOptions{Channel: x.Channel, DevMode: x.DevMode, JailMode: x.JailMode}
if strings.Contains(name, "/") || strings.HasSuffix(name, ".snap") || strings.Contains(name, ".snap.") {
installFromFile = true
changeID, err = cli.InstallPath(name, opts)
Expand Down
2 changes: 1 addition & 1 deletion cmd/snap/notes.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func (n *Notes) String() string {
ns = append(ns, "devmode")
} else if devmodeSnap {
// snap is devmode, but is not installed in devmode
ns = append(ns, "confined")
ns = append(ns, "jailmode")
}
} else if devmodeSnap {
ns = append(ns, n.Confinement)
Expand Down
4 changes: 2 additions & 2 deletions cmd/snap/notes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,10 @@ func (notesSuite) TestNotesDisabled(c *check.C) {
}).String(), check.Equals, "disabled")
}

func (notesSuite) TestNotesLocalConfined(c *check.C) {
func (notesSuite) TestNotesLocalJailMode(c *check.C) {
c.Check((&snap.Notes{
Local: true,
DevMode: false,
Confinement: "devmode",
}).String(), check.Equals, "confined")
}).String(), check.Equals, "jailmode")
}
59 changes: 28 additions & 31 deletions daemon/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ type snapInstruction struct {
Action string `json:"action"`
Channel string `json:"channel"`
DevMode bool `json:"devmode"`
Confined bool `json:"confined"`
JailMode bool `json:"jailmode"`
// dropping support temporarely until flag confusion is sorted,
// this isn't supported by client atm anyway
LeaveOld bool `json:"temp-dropped-leave-old"`
Expand Down Expand Up @@ -628,23 +628,33 @@ func withEnsureUbuntuCore(st *state.State, targetSnap string, userID int, instal
return []*state.TaskSet{ts}, nil
}

var errNoDevModeAndConfined = errors.New("cannot request both DevMode and Confined")
var errNoConfinedOnDevModeOS = errors.New("this system cannot install a confined snap")
var errNoDevModeAndJailMode = errors.New("cannot use devmode and jailmode flags together")
var errNoJailModeOnDevModeOS = errors.New("this system cannot honour the jailmode flag")

func snapInstall(inst *snapInstruction, st *state.State) (string, []*state.TaskSet, error) {
func mkFlags(devMode, jailMode bool) (snapstate.Flags, error) {
devModeOS := release.ReleaseInfo.ForceDevMode()
flags := snapstate.Flags(0)
isDevModeOS := release.ReleaseInfo.ForceDevMode()
if inst.DevMode || isDevModeOS {
flags |= snapstate.DevMode
}
if inst.Confined {
if isDevModeOS {
return "", nil, errNoConfinedOnDevModeOS
if jailMode {
if devModeOS {
return 0, errNoJailModeOnDevModeOS
}
if flags.DevMode() {
return "", nil, errNoDevModeAndConfined
if devMode {
return 0, errNoDevModeAndJailMode
}
flags |= snapstate.Confined
flags |= snapstate.JailMode
}
if devMode || devModeOS {
flags |= snapstate.DevMode
}

return flags, nil

}

func snapInstall(inst *snapInstruction, st *state.State) (string, []*state.TaskSet, error) {
flags, err := mkFlags(inst.DevMode, inst.JailMode)
if err != nil {
return "", nil, err
}

tsets, err := withEnsureUbuntuCore(st, inst.snap, inst.userID,
Expand Down Expand Up @@ -818,7 +828,7 @@ func trySnap(c *Command, r *http.Request, user *auth.UserState, trydir string, f
return AsyncResponse(nil, &Meta{Change: chg.ID()})
}

func isFormValueTrue(value []string) bool {
func isTrue(value []string) bool {
if len(value) == 0 {
return false
}
Expand Down Expand Up @@ -853,22 +863,9 @@ func sideloadSnap(c *Command, r *http.Request, user *auth.UserState) Response {
return BadRequest("cannot read POST form: %v", err)
}

var flags snapstate.Flags

if isFormValueTrue(form.Value["devmode"]) {
flags |= snapstate.DevMode
}
if isFormValueTrue(form.Value["confined"]) {
if flags.DevMode() {
return BadRequest(errNoDevModeAndConfined.Error())
}
flags |= snapstate.Confined
}
if release.ReleaseInfo.ForceDevMode() {
if flags.Confined() {
return BadRequest(errNoConfinedOnDevModeOS.Error())
}
flags |= snapstate.DevMode
flags, err := mkFlags(isTrue(form.Value["devmode"]), isTrue(form.Value["jailmode"]))
if err != nil {
return BadRequest(err.Error())
}

if len(form.Value["action"]) > 0 && form.Value["action"][0] == "try" {
Expand Down
48 changes: 24 additions & 24 deletions daemon/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -360,8 +360,8 @@ func (s *apiSuite) TestListIncludesAll(c *check.C) {
"maxReadBuflen",
"muxVars",
"errNothingToInstall",
"errNoDevModeAndConfined",
"errNoConfinedOnDevModeOS",
"errNoDevModeAndJailMode",
"errNoJailModeOnDevModeOS",
// snapInstruction vars:
"snapInstructionDispTable",
"snapstateInstall",
Expand Down Expand Up @@ -1256,33 +1256,33 @@ func (s *apiSuite) TestSideloadSnapDevMode(c *check.C) {
c.Check(chgSummary, check.Equals, `Install "local" snap from file "x"`)
}

func (s *apiSuite) TestSideloadSnapConfined(c *check.C) {
func (s *apiSuite) TestSideloadSnapJailMode(c *check.C) {
body := "" +
"----hello--\r\n" +
"Content-Disposition: form-data; name=\"snap\"; filename=\"x\"\r\n" +
"\r\n" +
"xyzzy\r\n" +
"----hello--\r\n" +
"Content-Disposition: form-data; name=\"confined\"\r\n" +
"Content-Disposition: form-data; name=\"jailmode\"\r\n" +
"\r\n" +
"true\r\n" +
"----hello--\r\n"
head := map[string]string{"Content-Type": "multipart/thing; boundary=--hello--"}
// try a multipart/form-data upload
restore := release.MockReleaseInfo(&release.OS{ID: "ubuntu"})
defer restore()
chgSummary := s.sideloadCheck(c, body, head, snapstate.Confined, true)
chgSummary := s.sideloadCheck(c, body, head, snapstate.JailMode, true)
c.Check(chgSummary, check.Equals, `Install "local" snap from file "x"`)
}

func (s *apiSuite) TestSideloadSnapConfinedAndDevmode(c *check.C) {
func (s *apiSuite) TestSideloadSnapJailModeAndDevmode(c *check.C) {
body := "" +
"----hello--\r\n" +
"Content-Disposition: form-data; name=\"snap\"; filename=\"x\"\r\n" +
"\r\n" +
"xyzzy\r\n" +
"----hello--\r\n" +
"Content-Disposition: form-data; name=\"confined\"\r\n" +
"Content-Disposition: form-data; name=\"jailmode\"\r\n" +
"\r\n" +
"true\r\n" +
"----hello--\r\n" +
Expand All @@ -1300,17 +1300,17 @@ func (s *apiSuite) TestSideloadSnapConfinedAndDevmode(c *check.C) {

rsp := sideloadSnap(snapsCmd, req, nil).(*resp)
c.Assert(rsp.Type, check.Equals, ResponseTypeError)
c.Check(rsp.Result.(*errorResult).Message, check.Equals, "cannot request both DevMode and Confined")
c.Check(rsp.Result.(*errorResult).Message, check.Equals, "cannot use devmode and jailmode flags together")
}

func (s *apiSuite) TestSideloadSnapConfinedInDevModeOS(c *check.C) {
func (s *apiSuite) TestSideloadSnapJailModeInDevModeOS(c *check.C) {
body := "" +
"----hello--\r\n" +
"Content-Disposition: form-data; name=\"snap\"; filename=\"x\"\r\n" +
"\r\n" +
"xyzzy\r\n" +
"----hello--\r\n" +
"Content-Disposition: form-data; name=\"confined\"\r\n" +
"Content-Disposition: form-data; name=\"jailmode\"\r\n" +
"\r\n" +
"true\r\n" +
"----hello--\r\n"
Expand All @@ -1327,7 +1327,7 @@ func (s *apiSuite) TestSideloadSnapConfinedInDevModeOS(c *check.C) {

rsp := sideloadSnap(snapsCmd, req, nil).(*resp)
c.Assert(rsp.Type, check.Equals, ResponseTypeError)
c.Check(rsp.Result.(*errorResult).Message, check.Equals, "this system cannot install a confined snap")
c.Check(rsp.Result.(*errorResult).Message, check.Equals, "this system cannot honour the jailmode flag")
}

func (s *apiSuite) TestSideloadSnapNotValidFormFile(c *check.C) {
Expand Down Expand Up @@ -1876,7 +1876,7 @@ func (s *apiSuite) TestInstallDevMode(c *check.C) {
c.Check(calledFlags&snapstate.DevMode, check.Equals, snapstate.Flags(snapstate.DevMode))
}

func (s *apiSuite) TestInstallConfined(c *check.C) {
func (s *apiSuite) TestInstallJailMode(c *check.C) {
var calledFlags snapstate.Flags

snapstateInstall = func(s *state.State, name, channel string, userID int, flags snapstate.Flags) (*state.TaskSet, error) {
Expand All @@ -1889,7 +1889,7 @@ func (s *apiSuite) TestInstallConfined(c *check.C) {
d := s.daemon(c)
inst := &snapInstruction{
Action: "install",
Confined: true,
JailMode: true,
}

st := d.overlord.State()
Expand All @@ -1898,39 +1898,39 @@ func (s *apiSuite) TestInstallConfined(c *check.C) {
_, _, err := inst.dispatch()(inst, st)
c.Check(err, check.IsNil)

c.Check(calledFlags&snapstate.Confined, check.Equals, snapstate.Flags(snapstate.Confined))
c.Check(calledFlags&snapstate.JailMode, check.Equals, snapstate.Flags(snapstate.JailMode))
}

func (s *apiSuite) TestInstallConfinedDevModeOS(c *check.C) {
func (s *apiSuite) TestInstallJailModeDevModeOS(c *check.C) {
restore := release.MockReleaseInfo(&release.OS{ID: "x-devmode-distro"})
defer restore()

d := s.daemon(c)
inst := &snapInstruction{
Action: "install",
Confined: true,
JailMode: true,
}

st := d.overlord.State()
st.Lock()
defer st.Unlock()
_, _, err := inst.dispatch()(inst, st)
c.Check(err, check.ErrorMatches, "this system cannot install a confined snap")
c.Check(err, check.ErrorMatches, "this system cannot honour the jailmode flag")
}

func (s *apiSuite) TestInstallConfinedDevMode(c *check.C) {
func (s *apiSuite) TestInstallJailModeDevMode(c *check.C) {
d := s.daemon(c)
inst := &snapInstruction{
Action: "install",
DevMode: true,
Confined: true,
JailMode: true,
}

st := d.overlord.State()
st.Lock()
defer st.Unlock()
_, _, err := inst.dispatch()(inst, st)
c.Check(err, check.ErrorMatches, "cannot request both DevMode and Confined")
c.Check(err, check.ErrorMatches, "cannot use devmode and jailmode flags together")
}

func snapList(rawSnaps interface{}) []map[string]interface{} {
Expand Down Expand Up @@ -3116,12 +3116,12 @@ func (s *apiSuite) TestBuyFailMissingParameter(c *check.C) {
})
}

func (s *apiSuite) TestIsFormValueTrue(c *check.C) {
c.Check(isFormValueTrue(nil), check.Equals, false)
func (s *apiSuite) TestIsTrue(c *check.C) {
c.Check(isTrue(nil), check.Equals, false)
for _, f := range []string{"", "false", "0", "False", "f", "try"} {
c.Check(isFormValueTrue([]string{f}), check.Equals, false, check.Commentf("expected %q to be false", f))
c.Check(isTrue([]string{f}), check.Equals, false, check.Commentf("expected %q to be false", f))
}
for _, t := range []string{"true", "1", "True", "t"} {
c.Check(isFormValueTrue([]string{t}), check.Equals, true, check.Commentf("expected %q to be true", t))
c.Check(isTrue([]string{t}), check.Equals, true, check.Commentf("expected %q to be true", t))
}
}
2 changes: 1 addition & 1 deletion overlord/snapstate/check_snap.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func checkSnap(st *state.State, snapFilePath string, curInfo *snap.Info, flags F
}

if s.NeedsDevMode() && !flags.DevModeAllowed() {
return fmt.Errorf("snap %q has devmode confinement but user has not requested nor overridden", s.Name())
return fmt.Errorf("snap %q requires devmode or confinement override", s.Name())
}

// verify we have a valid architecture
Expand Down
2 changes: 1 addition & 1 deletion overlord/snapstate/check_snap_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -270,5 +270,5 @@ confinement: devmode

err = snapstate.CheckSnap(nil, "snap-path", nil, 0)

c.Assert(err, ErrorMatches, ".* has devmode confinement but user has not requested nor overridden")
c.Assert(err, ErrorMatches, ".* requires devmode or confinement override")
}
Loading

0 comments on commit 5a1a52f

Please sign in to comment.