Skip to content

Commit

Permalink
daemon,o/servicestate: handle not-set scope and simplify error messages
Browse files Browse the repository at this point in the history
  • Loading branch information
@Meulengracht
Meulengracht committed Feb 21, 2024
1 parent 13209f5 commit 77437aa
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 68 deletions.
39 changes: 19 additions & 20 deletions daemon/api_apps_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ import (
"github.com/snapcore/snapd/snap"
"github.com/snapcore/snapd/systemd"
"github.com/snapcore/snapd/testutil"
"github.com/snapcore/snapd/wrappers"
)

var _ = check.Suite(&appsSuite{})
Expand Down Expand Up @@ -536,7 +535,7 @@ func (s *appsSuite) testPostAppsUser(c *check.C, inst servicestate.Instruction,
func (s *appsSuite) TestPostAppsStartOne(c *check.C) {
inst := servicestate.Instruction{Action: "start", Names: []string{"snap-a.svc2"}}
expected := []serviceControlArgs{
{action: "start", names: []string{"snap-a.svc2"}, scope: servicestate.ScopeSelector(wrappers.ServiceScopeAll)},
{action: "start", names: []string{"snap-a.svc2"}, scope: servicestate.ScopeSelector{"system", "user"}},
}
chg := s.testPostApps(c, inst, expected)
chg.State().Lock()
Expand All @@ -551,7 +550,7 @@ func (s *appsSuite) TestPostAppsStartOne(c *check.C) {
func (s *appsSuite) TestPostAppsStartTwo(c *check.C) {
inst := servicestate.Instruction{Action: "start", Names: []string{"snap-a"}}
expected := []serviceControlArgs{
{action: "start", names: []string{"snap-a.svc1", "snap-a.svc2"}, scope: servicestate.ScopeSelector(wrappers.ServiceScopeAll)},
{action: "start", names: []string{"snap-a.svc1", "snap-a.svc2"}, scope: servicestate.ScopeSelector{"system", "user"}},
}
chg := s.testPostApps(c, inst, expected)

Expand All @@ -567,7 +566,7 @@ func (s *appsSuite) TestPostAppsStartTwo(c *check.C) {
func (s *appsSuite) TestPostAppsStartThree(c *check.C) {
inst := servicestate.Instruction{Action: "start", Names: []string{"snap-a", "snap-b"}}
expected := []serviceControlArgs{
{action: "start", names: []string{"snap-a.svc1", "snap-a.svc2", "snap-b.svc3"}, scope: servicestate.ScopeSelector(wrappers.ServiceScopeAll)},
{action: "start", names: []string{"snap-a.svc1", "snap-a.svc2", "snap-b.svc3"}, scope: servicestate.ScopeSelector{"system", "user"}},
}
chg := s.testPostApps(c, inst, expected)
// check the summary expands the snap into actual apps
Expand All @@ -584,15 +583,15 @@ func (s *appsSuite) TestPostAppsStartThree(c *check.C) {
func (s *appsSuite) TestPostAppsStop(c *check.C) {
inst := servicestate.Instruction{Action: "stop", Names: []string{"snap-a.svc2"}}
expected := []serviceControlArgs{
{action: "stop", names: []string{"snap-a.svc2"}, scope: servicestate.ScopeSelector(wrappers.ServiceScopeAll)},
{action: "stop", names: []string{"snap-a.svc2"}, scope: servicestate.ScopeSelector{"system", "user"}},
}
s.testPostApps(c, inst, expected)
}

func (s *appsSuite) TestPostAppsRestart(c *check.C) {
inst := servicestate.Instruction{Action: "restart", Names: []string{"snap-a.svc2"}}
expected := []serviceControlArgs{
{action: "restart", names: []string{"snap-a.svc2"}, scope: servicestate.ScopeSelector(wrappers.ServiceScopeAll)},
{action: "restart", names: []string{"snap-a.svc2"}, scope: servicestate.ScopeSelector{"system", "user"}},
}
s.testPostApps(c, inst, expected)
}
Expand All @@ -601,7 +600,7 @@ func (s *appsSuite) TestPostAppsReload(c *check.C) {
inst := servicestate.Instruction{Action: "restart", Names: []string{"snap-a.svc2"}}
inst.Reload = true
expected := []serviceControlArgs{
{action: "restart", options: "reload", names: []string{"snap-a.svc2"}, scope: servicestate.ScopeSelector(wrappers.ServiceScopeAll)},
{action: "restart", options: "reload", names: []string{"snap-a.svc2"}, scope: servicestate.ScopeSelector{"system", "user"}},
}
s.testPostApps(c, inst, expected)
}
Expand All @@ -610,7 +609,7 @@ func (s *appsSuite) TestPostAppsEnableNow(c *check.C) {
inst := servicestate.Instruction{Action: "start", Names: []string{"snap-a.svc2"}}
inst.Enable = true
expected := []serviceControlArgs{
{action: "start", options: "enable", names: []string{"snap-a.svc2"}, scope: servicestate.ScopeSelector(wrappers.ServiceScopeAll)},
{action: "start", options: "enable", names: []string{"snap-a.svc2"}, scope: servicestate.ScopeSelector{"system", "user"}},
}
s.testPostApps(c, inst, expected)
}
Expand All @@ -619,7 +618,7 @@ func (s *appsSuite) TestPostAppsDisableNow(c *check.C) {
inst := servicestate.Instruction{Action: "stop", Names: []string{"snap-a.svc2"}}
inst.Disable = true
expected := []serviceControlArgs{
{action: "stop", options: "disable", names: []string{"snap-a.svc2"}, scope: servicestate.ScopeSelector(wrappers.ServiceScopeAll)},
{action: "stop", options: "disable", names: []string{"snap-a.svc2"}, scope: servicestate.ScopeSelector{"system", "user"}},
}
s.testPostApps(c, inst, expected)
}
Expand All @@ -641,7 +640,7 @@ func (s *appsSuite) TestPostAppsScopesUserAsRootNotAllowed(c *check.C) {
inst := servicestate.Instruction{
Action: "start",
Names: []string{"snap-a.svc1"},
Scope: servicestate.ScopeSelector(wrappers.ServiceScopeUser),
Scope: servicestate.ScopeSelector{"user"},
Users: servicestate.UserSelector{
Selector: servicestate.UserSelectionSelf,
},
Expand All @@ -661,22 +660,22 @@ func (s *appsSuite) TestPostAppsUsersAsRootHappy(c *check.C) {
inst := servicestate.Instruction{
Action: "start",
Names: []string{"snap-a.svc1"},
Scope: servicestate.ScopeSelector(wrappers.ServiceScopeUser),
Scope: servicestate.ScopeSelector{"user"},
Users: servicestate.UserSelector{
Selector: servicestate.UserSelectionAll,
},
}
expected := []serviceControlArgs{
// Expect no user to appear as we are not logged in
{action: "start", names: []string{"snap-a.svc1"}, scope: servicestate.ScopeSelector(wrappers.ServiceScopeUser)},
{action: "start", names: []string{"snap-a.svc1"}, scope: servicestate.ScopeSelector{"user"}},
}
s.testPostApps(c, inst, expected)
}

func (s *appsSuite) TestPostAppsScopesNotSpecifiedForRoot(c *check.C) {
inst := servicestate.Instruction{Action: "start", Names: []string{"snap-e.svc4"}}
expected := []serviceControlArgs{
{action: "start", names: []string{"snap-e.svc4"}, scope: servicestate.ScopeSelector(wrappers.ServiceScopeAll)},
{action: "start", names: []string{"snap-e.svc4"}, scope: servicestate.ScopeSelector{"system", "user"}},
}
s.testPostApps(c, inst, expected)
}
Expand All @@ -685,13 +684,13 @@ func (s *appsSuite) TestPostAppsUsersAsUserHappy(c *check.C) {
inst := servicestate.Instruction{
Action: "start",
Names: []string{"snap-a.svc1"},
Scope: servicestate.ScopeSelector(wrappers.ServiceScopeUser),
Scope: servicestate.ScopeSelector{"user"},
Users: servicestate.UserSelector{
Selector: servicestate.UserSelectionAll,
},
}
expected := []serviceControlArgs{
{action: "start", names: []string{"snap-a.svc1"}, scope: servicestate.ScopeSelector(wrappers.ServiceScopeUser)},
{action: "start", names: []string{"snap-a.svc1"}, scope: servicestate.ScopeSelector{"user"}},
}
s.testPostAppsUser(c, inst, expected, "")
}
Expand All @@ -716,7 +715,7 @@ func (s *appsSuite) TestPostAppsUsersUser(c *check.C) {
},
}
expected := []serviceControlArgs{
{action: "start", names: []string{"snap-a.svc1"}, scope: servicestate.ScopeSelector(wrappers.ServiceScopeSystem), users: []string{"username"}},
{action: "start", names: []string{"snap-a.svc1"}, scope: servicestate.ScopeSelector{"system"}, users: []string{"username"}},
}
s.testPostAppsUser(c, inst, expected, "")
}
Expand All @@ -731,21 +730,21 @@ func (s *appsSuite) TestPostAppsUsersWithUsernames(c *check.C) {
}
expected := []serviceControlArgs{
// Expect no user to appear as we are not logged in
{action: "start", names: []string{"snap-a.svc1"}, scope: servicestate.ScopeSelector(wrappers.ServiceScopeAll), users: []string{"my-user", "other-user"}},
{action: "start", names: []string{"snap-a.svc1"}, scope: servicestate.ScopeSelector{"system", "user"}, users: []string{"my-user", "other-user"}},
}
s.testPostApps(c, inst, expected)
}

func (s *appsSuite) TestPostAppsUserNotSpecifiedForRoot(c *check.C) {
inst := servicestate.Instruction{Action: "start", Names: []string{"snap-e.svc4"}, Scope: servicestate.ScopeSelector(wrappers.ServiceScopeSystem)}
inst := servicestate.Instruction{Action: "start", Names: []string{"snap-e.svc4"}, Scope: servicestate.ScopeSelector{"system"}}
expected := []serviceControlArgs{
{action: "start", names: []string{"snap-e.svc4"}, scope: servicestate.ScopeSelector(wrappers.ServiceScopeSystem)},
{action: "start", names: []string{"snap-e.svc4"}, scope: servicestate.ScopeSelector{"system"}},
}
s.testPostApps(c, inst, expected)
}

func (s *appsSuite) TestPostAppsUserNotSpecifiedForUser(c *check.C) {
inst := servicestate.Instruction{Action: "start", Names: []string{"snap-e.svc4"}, Scope: servicestate.ScopeSelector(wrappers.ServiceScopeUser)}
inst := servicestate.Instruction{Action: "start", Names: []string{"snap-e.svc4"}, Scope: servicestate.ScopeSelector{"user"}}
s.testPostAppsUser(c, inst, nil, "cannot perform operation on services: non-root users must specify users when targeting user services")
}

Expand Down
67 changes: 42 additions & 25 deletions overlord/servicestate/servicestate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,10 @@ func (us *UserSelector) UserList(currentUser *user.User) ([]string, error) {
return us.Names, nil
case UserSelectionSelf:
if currentUser == nil {
return nil, fmt.Errorf("internal error: for \"self\" the current user must be provided")
return nil, fmt.Errorf(`internal error: for "self" the current user must be provided`)
}
if currentUser.Uid == "0" {
return nil, fmt.Errorf("cannot use \"self\" for root user")
return nil, fmt.Errorf(`cannot use "self" for root user`)
}
return []string{currentUser.Username}, nil
case UserSelectionAll:
Expand Down Expand Up @@ -125,28 +125,26 @@ func (us *UserSelector) UnmarshalJSON(b []byte) error {
return nil
}

type ScopeSelector wrappers.ServiceScope
type ScopeSelector []string

func (ss *ScopeSelector) UnmarshalJSON(b []byte) error {
var s string
if err := json.Unmarshal(b, &s); err != nil {
return fmt.Errorf("cannot unmarshal, expected a string")
var scopes []string
if err := json.Unmarshal(b, &scopes); err != nil {
return fmt.Errorf("cannot unmarshal, expected a list of strings")
}

// XXX: Currently this will not allow non-root users to target both
// user/system daemons at once. Unless we do a similar trick like
// UserSelector or introduce a specific "all" so we can keep distinguishing
// between not-set and all.
switch s {
case "":
*ss = ScopeSelector(wrappers.ServiceScopeAll)
case "system":
*ss = ScopeSelector(wrappers.ServiceScopeSystem)
case "user":
*ss = ScopeSelector(wrappers.ServiceScopeUser)
default:
return fmt.Errorf(`cannot unmarshal, expected one of: "system", "user"`)
if len(scopes) > 2 {
return fmt.Errorf("unexpected number of scopes: %v", scopes)
}

for _, s := range scopes {
switch s {
case "system", "user":
default:
return fmt.Errorf(`cannot unmarshal, expected one of: "system", "user"`)
}
}
*ss = scopes
return nil
}

Expand All @@ -161,7 +159,23 @@ type Instruction struct {
}

func (i *Instruction) ServiceScope() wrappers.ServiceScope {
return wrappers.ServiceScope(i.Scope)
var hasUser, hasSystem bool
for _, opt := range i.Scope {
switch opt {
case "user":
hasUser = true
case "system":
hasSystem = true
}
}
switch {
case hasSystem && !hasUser:
return wrappers.ServiceScopeSystem
case hasUser && !hasSystem:
return wrappers.ServiceScopeUser
default:
return wrappers.ServiceScopeAll
}
}

func (i *Instruction) hasUserService(apps []*snap.AppInfo) bool {
Expand All @@ -178,16 +192,19 @@ func (i *Instruction) hasUserService(apps []*snap.AppInfo) bool {
// Make sure to call Instruction.Validate() before calling this.
func (i *Instruction) EnsureDefaultScopeForUser(u *user.User) {
// Set default scopes if not provided
if i.Scope == ScopeSelector(wrappers.ServiceScopeAll) {
// If non-root imply the service scope only to keep in line with backwards compatibility
if u.Uid != "0" {
i.Scope = ScopeSelector(wrappers.ServiceScopeSystem)
if len(i.Scope) == 0 {
// If root is making this request, implied scopes are all
if u.Uid == "0" {
i.Scope = ScopeSelector{"system", "user"}
} else {
// Otherwise imply the service scope only
i.Scope = ScopeSelector{"system"}
}
}
}

func (i *Instruction) validateScope(u *user.User, apps []*snap.AppInfo) error {
if i.Scope == ScopeSelector(wrappers.ServiceScopeAll) {
if len(i.Scope) == 0 {
// Providing no scope is only an issue for non-root users if the
// target is user-daemons.
if u.Uid != "0" && i.hasUserService(apps) {
Expand Down
Loading

0 comments on commit 77437aa

Please sign in to comment.