Ensure user store uses lowercased username for document id.

google38438 · Jun 24, 2019 · 9170356 · 9170356
1 parent 7671273
commit 9170356
Show file tree

Hide file tree

Showing 2 changed files with 98 additions and 12 deletions.
diff --git a/state/user.go b/state/user.go
@@ -37,12 +37,14 @@ func userIDFromGlobalKey(key string) string {
 }
 
 func (st *State) checkUserExists(name string) (bool, error) {
+	lowercaseName := strings.ToLower(name)
+
 	users, closer := st.db().GetCollection(usersC)
 	defer closer()
 
 	var count int
 	var err error
-	if count, err = users.FindId(name).Count(); err != nil {
+	if count, err = users.FindId(lowercaseName).Count(); err != nil {
 		return false, err
 	}
 	return count > 0, nil
@@ -72,13 +74,13 @@ func (st *State) addUser(name, displayName, password, creator string, secretKey
 	if !names.IsValidUserName(name) {
 		return nil, errors.Errorf("invalid user name %q", name)
 	}
-	nameToLower := strings.ToLower(name)
+	lowercaseName := strings.ToLower(name)
 
 	dateCreated := st.nowToTheSecond()
 	user := &User{
 		st: st,
 		doc: userDoc{
-			DocID:       nameToLower,
+			DocID:       lowercaseName,
 			Name:        name,
 			DisplayName: displayName,
 			SecretKey:   secretKey,
@@ -98,7 +100,7 @@ func (st *State) addUser(name, displayName, password, creator string, secretKey
 
 	ops := []txn.Op{{
 		C:      usersC,
-		Id:     nameToLower,
+		Id:     lowercaseName,
 		Assert: txn.DocMissing,
 		Insert: &user.doc,
 	}}
@@ -126,7 +128,7 @@ func (st *State) addUser(name, displayName, password, creator string, secretKey
 // RemoveUser marks the user as deleted. This obviates the ability of a user
 // to function, but keeps the userDoc retaining provenance, i.e. auditing.
 func (st *State) RemoveUser(tag names.UserTag) error {
-	name := strings.ToLower(tag.Name())
+	lowercaseName := strings.ToLower(tag.Name())
 
 	u, err := st.User(tag)
 	if err != nil {
@@ -144,7 +146,7 @@ func (st *State) RemoveUser(tag names.UserTag) error {
 			}
 		}
 		ops := []txn.Op{{
-			Id:     name,
+			Id:     lowercaseName,
 			C:      usersC,
 			Assert: txn.DocExists,
 			Update: bson.M{"$set": bson.M{"deleted": true}},
@@ -155,9 +157,9 @@ func (st *State) RemoveUser(tag names.UserTag) error {
 }
 
 func createInitialUserOps(controllerUUID string, user names.UserTag, password, salt string, dateCreated time.Time) []txn.Op {
-	nameToLower := strings.ToLower(user.Name())
+	lowercaseName := strings.ToLower(user.Name())
 	doc := userDoc{
-		DocID:        nameToLower,
+		DocID:        lowercaseName,
 		Name:         user.Name(),
 		DisplayName:  user.Name(),
 		PasswordHash: utils.UserPasswordHash(password, salt),
@@ -167,7 +169,7 @@ func createInitialUserOps(controllerUUID string, user names.UserTag, password, s
 	}
 	ops := []txn.Op{{
 		C:      usersC,
-		Id:     nameToLower,
+		Id:     lowercaseName,
 		Assert: txn.DocMissing,
 		Insert: &doc,
 	}}
@@ -428,9 +430,10 @@ func (u *User) SetPasswordHash(pwHash string, pwSalt string) error {
 			bson.DocElem{"$unset", bson.D{{"secretkey", ""}}},
 		)
 	}
+	lowercaseName := strings.ToLower(u.Name())
 	ops := []txn.Op{{
 		C:      usersC,
-		Id:     u.Name(),
+		Id:     lowercaseName,
 		Assert: txn.DocExists,
 		Update: update,
 	}}
@@ -494,9 +497,10 @@ func (u *User) Enable() error {
 }
 
 func (u *User) setDeactivated(value bool) error {
+	lowercaseName := strings.ToLower(u.Name())
 	ops := []txn.Op{{
 		C:      usersC,
-		Id:     u.Name(),
+		Id:     lowercaseName,
 		Assert: txn.DocExists,
 		Update: bson.D{{"$set", bson.D{{"deactivated", value}}}},
 	}}
@@ -575,9 +579,10 @@ func (u *User) ResetPassword() ([]byte, error) {
 				},
 			},
 		}
+		lowercaseName := strings.ToLower(u.Name())
 		return []txn.Op{{
 			C:      usersC,
-			Id:     u.Name(),
+			Id:     lowercaseName,
 			Assert: txn.DocExists,
 			Update: update,
 		}}, nil

diff --git a/state/user_test.go b/state/user_test.go
@@ -6,6 +6,7 @@ package state_test
 import (
 	"fmt"
 	"regexp"
+	"strings"
 	"time"
 
 	"github.com/juju/errors"
@@ -83,6 +84,9 @@ func (s *UserSuite) TestCheckUserExists(c *gc.C) {
 	exists, err := state.CheckUserExists(s.State, user.Name())
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(exists, jc.IsTrue)
+	exists, err = state.CheckUserExists(s.State, strings.ToUpper(user.Name()))
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(exists, jc.IsTrue)
 	exists, err = state.CheckUserExists(s.State, "notAUser")
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(exists, jc.IsFalse)
@@ -228,6 +232,30 @@ func (s *UserSuite) TestRemoveUser(c *gc.C) {
 	c.Check(err, gc.ErrorMatches, `user "username-\d+" is permanently deleted`)
 }
 
+func (s *UserSuite) TestRemoveUserUppercaseName(c *gc.C) {
+	name := "NameWithUppercase"
+	user := s.Factory.MakeUser(c, &factory.UserParams{
+		Name:     name,
+		Password: "wow very sea cret",
+	})
+
+	// Assert user exists and can authenticate.
+	c.Assert(user.PasswordValid("wow very sea cret"), jc.IsTrue)
+
+	// Look for the user.
+	u, err := s.State.User(user.UserTag())
+	c.Check(err, jc.ErrorIsNil)
+	c.Assert(u, jc.DeepEquals, user)
+
+	// Remove the user.
+	err = s.State.RemoveUser(user.UserTag())
+	c.Check(err, jc.ErrorIsNil)
+
+	// Check to verify the user cannot be retrieved.
+	u, err = s.State.User(user.UserTag())
+	c.Check(err, gc.ErrorMatches, fmt.Sprintf(`user "%s" is permanently deleted`, name))
+}
+
 func (s *UserSuite) TestRemoveUserRemovesUserAccess(c *gc.C) {
 	user := s.Factory.MakeUser(c, &factory.UserParams{Password: "so sekrit"})
 
@@ -279,6 +307,25 @@ func (s *UserSuite) TestDisableUser(c *gc.C) {
 	c.Assert(s.activeUsers(c), jc.DeepEquals, []string{"test-admin", user.Name()})
 }
 
+func (s *UserSuite) TestDisableUserUppercaseName(c *gc.C) {
+	name := "NameWithUppercase"
+	user := s.Factory.MakeUser(c, &factory.UserParams{Password: "a-password", Name: name})
+	c.Assert(user.IsDisabled(), jc.IsFalse)
+	c.Assert(s.activeUsers(c), jc.DeepEquals, []string{name, "test-admin"})
+
+	err := user.Disable()
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(user.IsDisabled(), jc.IsTrue)
+	c.Assert(user.PasswordValid("a-password"), jc.IsFalse)
+	c.Assert(s.activeUsers(c), jc.DeepEquals, []string{"test-admin"})
+
+	err = user.Enable()
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(user.IsDisabled(), jc.IsFalse)
+	c.Assert(user.PasswordValid("a-password"), jc.IsTrue)
+	c.Assert(s.activeUsers(c), jc.DeepEquals, []string{name, "test-admin"})
+}
+
 func (s *UserSuite) TestDisableUserDisablesUserAccess(c *gc.C) {
 	user := s.Factory.MakeUser(c, &factory.UserParams{Password: "so sekrit"})
 
@@ -356,6 +403,27 @@ func (s *UserSuite) TestSetPasswordHash(c *gc.C) {
 	c.Assert(user.PasswordValid("foo-12345678901234567890"), jc.IsFalse)
 }
 
+func (s *UserSuite) TestSetPasswordHashUppercaseName(c *gc.C) {
+	name := "NameWithUppercase"
+	user := s.Factory.MakeUser(c, &factory.UserParams{Name: name})
+
+	salt, err := utils.RandomSalt()
+	c.Assert(err, jc.ErrorIsNil)
+	err = user.SetPasswordHash(utils.UserPasswordHash("foo", salt), salt)
+	c.Assert(err, jc.ErrorIsNil)
+
+	c.Assert(user.PasswordValid("foo"), jc.IsTrue)
+	c.Assert(user.PasswordValid("bar"), jc.IsFalse)
+
+	// User passwords should *not* use the fast PasswordHash function
+	hash := utils.AgentPasswordHash("foo-12345678901234567890")
+	c.Assert(err, jc.ErrorIsNil)
+	err = user.SetPasswordHash(hash, "")
+	c.Assert(err, jc.ErrorIsNil)
+
+	c.Assert(user.PasswordValid("foo-12345678901234567890"), jc.IsFalse)
+}
+
 func (s *UserSuite) TestSetPasswordHashWithSalt(c *gc.C) {
 	user := s.Factory.MakeUser(c, nil)
 
@@ -469,6 +537,19 @@ func (s *UserSuite) TestResetPassword(c *gc.C) {
 	c.Assert(u.SecretKey(), gc.DeepEquals, key)
 }
 
+func (s *UserSuite) TestResetPasswordUppercaseName(c *gc.C) {
+	u, err := s.State.AddUserWithSecretKey("BobHasAnUppercaseName", "display", "admin")
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(u.SecretKey(), gc.HasLen, 32)
+	oldKey := u.SecretKey()
+
+	key, err := u.ResetPassword()
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(key, gc.Not(gc.DeepEquals), oldKey)
+	c.Assert(key, gc.NotNil)
+	c.Assert(u.SecretKey(), gc.DeepEquals, key)
+}
+
 func (s *UserSuite) TestResetPasswordFailIfDeactivated(c *gc.C) {
 	u, err := s.State.AddUser("bob", "display", "pass", "admin")
 	c.Assert(err, jc.ErrorIsNil)