Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JUJU-3333] Users removal triggers the removal of model access #15324

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 30 additions & 10 deletions state/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,6 @@ func (st *State) updateExistingUser(u *User, displayName, password, creator stri
}},
)
}
// remove previous controller permissions
removeControllerOps := removeControllerUserOps(st.ControllerUUID(), u.UserTag())

// create default new ones
createControllerOps := createControllerUserOps(st.ControllerUUID(),
Expand All @@ -186,7 +184,6 @@ func (st *State) updateExistingUser(u *User, displayName, password, creator stri
Assert: txn.DocExists,
Update: updateUser,
}}
updateUserOps = append(updateUserOps, removeControllerOps...)
updateUserOps = append(updateUserOps, createControllerOps...)

if err := u.st.db().RunTransaction(updateUserOps); err != nil {
Expand Down Expand Up @@ -215,6 +212,28 @@ func (st *State) RemoveUser(tag names.UserTag) error {
return nil
}

// remove the access to all the models and the current controller
// first query all the models for this user
modelsSummary, err := st.ModelSummariesForUser(tag, true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not how you would iterate across all the model users.

if err != nil {
return err
}

for _, summary := range modelsSummary {
// remove all the access permissions
removeModelOps := removeModelUserOps(summary.UUID, tag)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removeModelUserOps can't be run like this. You'll need to manually write the ops for this, as the removeModelUserOps are meant to be run from the state opened for a specific model, so you'll need to handle model prefixed ids manually. See ensureModelUUID and modelStateCollection and GetCollection.

if err := st.db().RunTransactionFor(summary.UUID, removeModelOps); err != nil {
logger.Errorf("impossible to remove user from models", err)
return err
}
}

// remove the user from the user controllers
if err := st.removeControllerUser(tag); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should actually be getting this user from state first to check it still exists.

logger.Errorf("impossible to remove user from controller", err)
return err
}

dateRemoved := st.nowToTheSecond()
// new entry in the removal log
newRemovalLogEntry := userRemovedLogEntry{
Expand All @@ -235,13 +254,14 @@ func (st *State) RemoveUser(tag names.UserTag) error {
return nil, errors.Trace(err)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be building all of those ops and txns above into this buildTxn function so they can be done together.

ops := []txn.Op{{
Id: lowercaseName,
C: usersC,
Assert: txn.DocExists,
Update: bson.M{"$set": bson.M{
"deleted": true, "removallog": removalLog}},
}}
ops := []txn.Op{
{
Id: lowercaseName,
C: usersC,
Assert: txn.DocExists,
Update: bson.M{"$set": bson.M{
"deleted": true, "removallog": removalLog}}},
}
return ops, nil
}
return st.db().Run(buildTxn)
Expand Down
45 changes: 45 additions & 0 deletions state/user_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,51 @@ func (s *UserSuite) TestRemoveUserRemovesUserAccess(c *gc.C) {
c.Assert(err, gc.ErrorMatches, fmt.Sprintf("user %q is permanently deleted", user.UserTag().Name()))
}

func (s *UserSuite) TestRecreatedUsersResetPermissions(c *gc.C) {
user := s.Factory.MakeUser(c, &factory.UserParams{Password: "so sekrit"})

// Assert user exists and can authenticate.
c.Assert(user.PasswordValid("so sekrit"), jc.IsTrue)

s.State.SetUserAccess(user.UserTag(), s.Model.ModelTag(), permission.AdminAccess)
s.State.SetUserAccess(user.UserTag(), s.State.ControllerTag(), permission.SuperuserAccess)

uam, err := s.State.UserAccess(user.UserTag(), s.Model.ModelTag())
c.Check(err, jc.ErrorIsNil)
c.Check(uam.Access, gc.Equals, permission.AdminAccess)

uac, err := s.State.UserAccess(user.UserTag(), s.State.ControllerTag())
c.Check(err, jc.ErrorIsNil)
c.Check(uac.Access, gc.Equals, permission.SuperuserAccess)

// Look for the user.
u, err := s.State.User(user.UserTag())
c.Check(err, jc.ErrorIsNil)
c.Check(u, jc.DeepEquals, user)

// Remove the user.
err = s.State.RemoveUser(user.UserTag())
c.Check(err, jc.ErrorIsNil)

// Add the user again with other password and access
userRecreated := s.Factory.MakeUser(c, &factory.UserParams{
Password: "otherpassword",
Access: permission.ReadAccess})

// Assert user exists and can authenticate.
c.Assert(userRecreated.PasswordValid("otherpassword"), jc.IsTrue)

// Check that the recreated user does not have the permissions set previously
urac, err := s.State.UserAccess(userRecreated.UserTag(), s.State.ControllerTag())
c.Check(err, jc.ErrorIsNil)
c.Check(urac.Access, gc.Equals, permission.LoginAccess)

// No model access was set yet
uram, err := s.State.UserAccess(userRecreated.UserTag(), s.Model.ModelTag())
c.Assert(err, jc.ErrorIsNil)
c.Check(uram.Access, gc.Equals, permission.ReadAccess)
}

func (s *UserSuite) TestDisableUser(c *gc.C) {
user := s.Factory.MakeUser(c, &factory.UserParams{Password: "a-password"})
c.Assert(user.IsDisabled(), jc.IsFalse)
Expand Down