Merge pull request #1647 from niemeyer/mvo5-bugfix-create-user-fixes

many: various fixes around the `create-user` command

daemon/api.go

@@ -1415,7 +1415,7 @@ func abortChange(c *Command, r *http.Request, user *auth.UserState) Response {
 var (
 	postCreateUserUcrednetGetUID = ucrednetGetUID
 	storeUserInfo                = store.UserInfo
-	osutilAddExtraUser           = osutil.AddExtraUser
+	osutilAddExtraSudoUser       = osutil.AddExtraSudoUser
 )
 
 func postCreateUser(c *Command, r *http.Request, user *auth.UserState) Response {
@@ -1444,8 +1444,12 @@ func postCreateUser(c *Command, r *http.Request, user *auth.UserState) Response
 	if err != nil {
 		return BadRequest("cannot create user %q: %s", createData.EMail, err)
 	}
+	if len(v.SSHKeys) == 0 {
+		return BadRequest("cannot create user for %s: no ssh keys found", createData.EMail)
+	}
 
-	if err := osutilAddExtraUser(v.Username, v.SSHKeys); err != nil {
+	gecos := fmt.Sprintf("%s,%s", createData.EMail, v.OpenIDIdentifier)
+	if err := osutilAddExtraSudoUser(v.Username, v.SSHKeys, gecos); err != nil {
 		return BadRequest("cannot create user %s: %s", v.Username, err)
 	}
 

daemon/api_test.go

@@ -388,7 +388,7 @@ func (s *apiSuite) TestListIncludesAll(c *check.C) {
 		"snapstateTryPath",
 		"snapstateGet",
 		"readSnapInfo",
-		"osutilAddExtraUser",
+		"osutilAddExtraSudoUser",
 		"storeUserInfo",
 		"postCreateUserUcrednetGetUID",
 		"ensureStateSoon",
@@ -3076,25 +3076,52 @@ func (s *apiSuite) TestStateChangeAbortIsReady(c *check.C) {
 	})
 }
 
+func (s *apiSuite) TestPostCreateUserNoSSHKeys(c *check.C) {
+	storeUserInfo = func(user string) (*store.User, error) {
+		c.Check(user, check.Equals, "popper@lse.ac.uk")
+		return &store.User{
+			Username:         "karl",
+			OpenIDIdentifier: "xxyyzz",
+		}, nil
+	}
+	postCreateUserUcrednetGetUID = func(string) (uint32, error) {
+		return 0, nil
+	}
+	defer func() {
+		postCreateUserUcrednetGetUID = ucrednetGetUID
+	}()
+
+	buf := bytes.NewBufferString(`{"email": "popper@lse.ac.uk"}`)
+	req, err := http.NewRequest("POST", "/v2/create-user", buf)
+	c.Assert(err, check.IsNil)
+
+	rsp := postCreateUser(createUserCmd, req, nil).(*resp)
+
+	c.Check(rsp.Type, check.Equals, ResponseTypeError)
+	c.Check(rsp.Result.(*errorResult).Message, check.Matches, "cannot create user for popper@lse.ac.uk: no ssh keys found")
+}
+
 func (s *apiSuite) TestPostCreateUser(c *check.C) {
 	storeUserInfo = func(user string) (*store.User, error) {
 		c.Check(user, check.Equals, "popper@lse.ac.uk")
 		return &store.User{
-			Username: "karl",
-			SSHKeys:  []string{"ssh1", "ssh2"},
+			Username:         "karl",
+			SSHKeys:          []string{"ssh1", "ssh2"},
+			OpenIDIdentifier: "xxyyzz",
 		}, nil
 	}
-	osutilAddExtraUser = func(username string, sshKeys []string) error {
+	osutilAddExtraSudoUser = func(username string, sshKeys []string, gecos string) error {
 		c.Check(username, check.Equals, "karl")
 		c.Check(sshKeys, check.DeepEquals, []string{"ssh1", "ssh2"})
+		c.Check(gecos, check.Equals, "popper@lse.ac.uk,xxyyzz")
 		return nil
 	}
 
 	postCreateUserUcrednetGetUID = func(string) (uint32, error) {
 		return 0, nil
 	}
 	defer func() {
-		osutilAddExtraUser = osutil.AddExtraUser
+		osutilAddExtraSudoUser = osutil.AddExtraSudoUser
 		postCreateUserUcrednetGetUID = ucrednetGetUID
 	}()
 

osutil/user.go

@@ -26,15 +26,28 @@ import (
 	"os/exec"
 	"os/user"
 	"path/filepath"
+	"regexp"
 	"strings"
 )
 
 var userLookup = user.Lookup
 
-func AddExtraUser(name string, sshKeys []string) error {
-	// FIXME: put date/time/openid in there
-	gecos := "created by snapd"
-	cmd := exec.Command("adduser", "--gecos", gecos, "--extrausers", "--disabled-password", name)
+func AddExtraSudoUser(name string, sshKeys []string, gecos string) error {
+	// we check the (user)name ourselves, adduser is a bit too
+	// strict (i.e. no `.`) - this regexp is in sync with that SSO
+	// allows as valid usernames
+	validNames := regexp.MustCompile(`^[a-z0-9][-a-z0-9+.-_]*$`)
+	if !validNames.MatchString(name) {
+		return fmt.Errorf("cannot add user %q: name contains invalid characters", name)
+	}
+
+	cmd := exec.Command("adduser",
+		"--force-badname",
+		"--gecos", gecos,
+		"--extrausers",
+		"--disabled-password",
+		"--add_extra_groups", "sudo",
+		name)
 	if output, err := cmd.CombinedOutput(); err != nil {
 		return fmt.Errorf("adduser failed with %s: %s", err, output)
 	}

osutil/user_test.go

@@ -36,7 +36,7 @@ type createUserSuite struct {
 
 var _ = check.Suite(&createUserSuite{})
 
-func (s *createUserSuite) TestAddExtraUser(c *check.C) {
+func (s *createUserSuite) TestAddExtraSudoUser(c *check.C) {
 	mockHome := c.MkDir()
 	restorer := osutil.MockUserLookup(func(string) (*user.User, error) {
 		return &user.User{
@@ -48,12 +48,17 @@ func (s *createUserSuite) TestAddExtraUser(c *check.C) {
 	mc := testutil.MockCommand(c, "adduser", "true")
 	defer mc.Restore()
 
-	err := osutil.AddExtraUser("karl", []string{"ssh-key1", "ssh-key2"})
+	err := osutil.AddExtraSudoUser("karl", []string{"ssh-key1", "ssh-key2"}, "my gecos")
 	c.Assert(err, check.IsNil)
 	c.Check(mc.Calls(), check.DeepEquals, [][]string{
-		{"adduser", "--gecos", "created by snapd", "--extrausers", "--disabled-password", "karl"},
+		{"adduser", "--force-badname", "--gecos", "my gecos", "--extrausers", "--disabled-password", "--add_extra_groups", "sudo", "karl"},
 	})
 	sshKeys, err := ioutil.ReadFile(filepath.Join(mockHome, ".ssh", "authorized_keys"))
 	c.Assert(err, check.IsNil)
 	c.Check(string(sshKeys), check.Equals, "ssh-key1\nssh-key2")
 }
+
+func (s *createUserSuite) TestAddExtraSudoUserInvalid(c *check.C) {
+	err := osutil.AddExtraSudoUser("k!", nil, "my gecos")
+	c.Assert(err, check.ErrorMatches, `cannot add user "k!": name contains invalid characters`)
+}