[JUJU-3333] Users removal triggers the removal of model access #15324
Conversation
… the user is registered in that controller.
| @@ -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) | |||
There was a problem hiding this comment.
This is not how you would iterate across all the model users.
|
|
||
| for _, summary := range modelsSummary { | ||
| // remove all the access permissions | ||
| removeModelOps := removeModelUserOps(summary.UUID, tag) |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // remove the user from the user controllers | ||
| if err := st.removeControllerUser(tag); err != nil { |
There was a problem hiding this comment.
You should actually be getting this user from state first to check it still exists.
| @@ -235,13 +254,14 @@ func (st *State) RemoveUser(tag names.UserTag) error { | |||
| return nil, errors.Trace(err) | |||
| } | |||
| } | |||
There was a problem hiding this comment.
You should be building all of those ops and txns above into this buildTxn function so they can be done together.
|
Please address my comment in this one here #15279 |
…_user_access #15395 This PR ports the fix for the broken user access reported in #15351 and #15324 ## Checklist *If an item is not applicable, use `~strikethrough~`.* - [ ] Code style: imports ordered, good names, simple structure, etc - [ ] Comments saying why design decisions were made - [ ] Go unit tests, with comments saying what you're testing - [ ] [Integration tests](https://github.com/juju/juju/tree/develop/tests), with comments saying what you're testing - [ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages ## QA steps Refer to #15324 for QA steps.
After enabling user names to be reused (#15266 ) it has been observed that for those recreated users that where previously granted with access permissions to other models, these permissions are automatically recreated. This is a misbehaviour that is corrected in this PR by removing all the permissions a user is granted when the user is removed from the controller. This makes possible to have a clear list of permissions when a user is recreated avoiding this unexpected behaviour.
Checklist
If an item is not applicable, use
~strikethrough~.- [ ] Code style: imports ordered, good names, simple structure, etc- [ ] Comments saying why design decisions were made- [ ] Integration tests, with comments saying what you're testing- [ ] doc.go added or updated in changed packagesQA steps
Try to replicate an scenario for a recreated user who was granted some permissions.
Grant user access
Destroy the user and recreate
Now if you check the users for model
test, no permissions are set