Skip to content

Commit

Permalink
Merge pull request juju#10369 from hpidcock/1833449-register-uppercas…
Browse files Browse the repository at this point in the history 
…e-username

juju#10369

## Description of change

Not all user store operations correctly formatted the mongo document id.
This fixes the behaviour in all affected user state functions and add tests to ensure this behaviour works.

Previously `juju register` with a secret token failed because the update password didn't behave correctly when a username contained uppercase.

## QA steps

Adding a user and registering that user should succeed when the username contains uppercase.
```
juju bootstrap localhost
juju add-user TestName
XDG_DATA_HOME=`mktemp -d` juju register <secret token from add-user>`
```

## Documentation changes

N/A

## Bug reference

[https://bugs.launchpad.net/juju/+bug/1833449](https://bugs.launchpad.net/juju/+bug/1833449)
  • Loading branch information
@jujubot
jujubot authored Jun 24, 2019
2 parents cfa6597 + 9170356 commit ff42def
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 12 deletions.
29 changes: 17 additions & 12 deletions state/user.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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,
}}
Expand Down Expand Up @@ -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 {
Expand All @@ -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}},
Expand All @@ -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),
Expand All @@ -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,
}}
Expand Down Expand Up @@ -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,
}}
Expand Down Expand Up @@ -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}}}},
}}
Expand Down Expand Up @@ -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
Expand Down
81 changes: 81 additions & 0 deletions state/user_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package state_test
import (
"fmt"
"regexp"
"strings"
"time"

"github.com/juju/errors"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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"})

Expand Down Expand Up @@ -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"})

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit ff42def

Please sign in to comment.