Make user management messages more informative

[reedobrien]

Description of change

Why is this change needed?

This reworks the error messages when adding and removing users. It
essentially returns "user unavailable" rather than "not found" when
deleting and "is permanently deleted" rather than "user exists".

It also changes a bunch of gocheck asserts into checks where is made no
sense to fail the suite on the first error.

QA steps

How do we verify that the change works?

1. Bootstrap a controller, say lxd juju bootstrap lxd lp/1630728
2. Add a user:

$ juju add-user jjam
User "jjam" added
Please send this command to jjam:
    juju register snip...

"jjam" has not been granted access to any models. You can use "juju grant" to grant access.

3. Try adding it again and ensure it says unavailable rather than not found

juju add-user jjaM
ERROR failed to create user: username unavailable

4. Delete the user and verify the confirmation message is informative.

$ juju remove-user jjam
WARNING! This command will permanently archive the user "jjam" on the "lp/1630728_remove-user"
controller.

This action is irreversible. If you wish to temporarily disable the
user please use the `juju disable-user` command. See
 `juju help disable-user` for more details.

Continue (y/N)? y
User "jjam" removed

5. Try to remove the user again and ensure we have unavailable instead of not found.

$ juju remove-user jjam -y
ERROR failed to delete user "jjam": the user has already been permanently deleted

6. Try to add a user with the same name

$ juju add-user jjaM
ERROR failed to create user: username unavailable

Documentation changes

Does it affect current user workflow? CLI? API?
It changes some output but not input.

Bug reference

Does this change fix a bug? Please add a link to it.

Refs: https://bugs.launchpad.net/juju/+bug/1630728

[jamesbeedy]

@reedobrien this is great! +1

[jameinel]

So it sounds like your patch just makes the errors nicer, which is good. But is it true that once you've created a user and then deleted it, that name can no longer ever be used again? That doesn't seem like a good interaction. John =:->

…

On Feb 25, 2017 07:16, "reedobrien" ***@***.***> wrote: Description of change Why is this change needed? This reworks the error messages when adding and removing users. It essentially returns "user unavailable" rather than "not found" when deleting and "is permanently deleted" rather than "user exists". It also changes a bunch of gocheck asserts into checks where is made no sense to fail the suite on the first error. QA steps How do we verify that the change works? 1. Bootstrap a controller, say lxd juju bootstrap lxd lp/1630728 2. Add a user: $ juju add-user jjam User "jjam" added Please send this command to jjam: juju register snip... "jjam" has not been granted access to any models. You can use "juju grant" to grant access. 1. Try adding it again and ensure it says unavailable rather than not found juju add-user jjaM ERROR failed to create user: username unavailable 1. Delete the user and verify the confirmation message is informative. $ juju remove-user jjam WARNING! This command will permanently archive the user "jjam" on the "lp/1630728_remove-user" controller. This action is irreversible. If you wish to temporarily disable the user please use the `juju disable-user` command. See `juju help disable-user` for more details. Continue (y/N)? y User "jjam" removed 1. Try to remove the user again and ensure we have unavailable instead of not found. $ juju remove-user jjam -y ERROR failed to delete user "jjam": the user has already been permanently deleted 1. Try to add a user with the same name $ juju add-user jjaM ERROR failed to create user: username unavailable Documentation changes Does it affect current user workflow? CLI? API? It changes some output but not input. Bug reference Does this change fix a bug? Please add a link to it. Refs: https://bugs.launchpad.net/juju/+bug/1630728 ------------------------------ You can view, comment on, or merge this pull request online at: #7035 Commit Summary - Make user management messages more informative File Changes - *M* api/usermanager/client_test.go <https://github.com/juju/juju/pull/7035/files#diff-0> (4) - *M* apiserver/usermanager/usermanager_test.go <https://github.com/juju/juju/pull/7035/files#diff-1> (4) - *M* cmd/juju/user/remove.go <https://github.com/juju/juju/pull/7035/files#diff-2> (18) - *M* cmd/juju/user/remove_test.go <https://github.com/juju/juju/pull/7035/files#diff-3> (10) - *M* featuretests/cmd_juju_user_test.go <https://github.com/juju/juju/pull/7035/files#diff-4> (9) - *M* state/model.go <https://github.com/juju/juju/pull/7035/files#diff-5> (10) - *M* state/user.go <https://github.com/juju/juju/pull/7035/files#diff-6> (9) - *M* state/user_test.go <https://github.com/juju/juju/pull/7035/files#diff-7> (41) Patch Links: - https://github.com/juju/juju/pull/7035.patch - https://github.com/juju/juju/pull/7035.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#7035>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMYfXzaupn6hjAQZMa8ope4HumD3hIKks5rf50egaJpZM4ML4in> .

[jameinel]

(It doesn't have to happen with this patch, but it does seem like something we should address.) John =:->

…

On Feb 26, 2017 07:00, "John Meinel" ***@***.***> wrote: So it sounds like your patch just makes the errors nicer, which is good. But is it true that once you've created a user and then deleted it, that name can no longer ever be used again? That doesn't seem like a good interaction. John =:-> On Feb 25, 2017 07:16, "reedobrien" ***@***.***> wrote: > Description of change > > Why is this change needed? > > This reworks the error messages when adding and removing users. It > essentially returns "user unavailable" rather than "not found" when > deleting and "is permanently deleted" rather than "user exists". > > It also changes a bunch of gocheck asserts into checks where is made no > sense to fail the suite on the first error. > QA steps > > How do we verify that the change works? > > 1. Bootstrap a controller, say lxd juju bootstrap lxd lp/1630728 > 2. Add a user: > > $ juju add-user jjam > User "jjam" added > Please send this command to jjam: > juju register snip... > > "jjam" has not been granted access to any models. You can use "juju grant" to grant access. > > > 1. Try adding it again and ensure it says unavailable rather than not > found > > juju add-user jjaM > ERROR failed to create user: username unavailable > > > 1. Delete the user and verify the confirmation message is informative. > > $ juju remove-user jjam > WARNING! This command will permanently archive the user "jjam" on the "lp/1630728_remove-user" > controller. > > This action is irreversible. If you wish to temporarily disable the > user please use the `juju disable-user` command. See > `juju help disable-user` for more details. > > Continue (y/N)? y > User "jjam" removed > > > 1. Try to remove the user again and ensure we have unavailable > instead of not found. > > $ juju remove-user jjam -y > ERROR failed to delete user "jjam": the user has already been permanently deleted > > > 1. Try to add a user with the same name > > $ juju add-user jjaM > ERROR failed to create user: username unavailable > > Documentation changes > > Does it affect current user workflow? CLI? API? > It changes some output but not input. > Bug reference > > Does this change fix a bug? Please add a link to it. > > Refs: https://bugs.launchpad.net/juju/+bug/1630728 > ------------------------------ > You can view, comment on, or merge this pull request online at: > > #7035 > Commit Summary > > - Make user management messages more informative > > File Changes > > - *M* api/usermanager/client_test.go > <https://github.com/juju/juju/pull/7035/files#diff-0> (4) > - *M* apiserver/usermanager/usermanager_test.go > <https://github.com/juju/juju/pull/7035/files#diff-1> (4) > - *M* cmd/juju/user/remove.go > <https://github.com/juju/juju/pull/7035/files#diff-2> (18) > - *M* cmd/juju/user/remove_test.go > <https://github.com/juju/juju/pull/7035/files#diff-3> (10) > - *M* featuretests/cmd_juju_user_test.go > <https://github.com/juju/juju/pull/7035/files#diff-4> (9) > - *M* state/model.go > <https://github.com/juju/juju/pull/7035/files#diff-5> (10) > - *M* state/user.go > <https://github.com/juju/juju/pull/7035/files#diff-6> (9) > - *M* state/user_test.go > <https://github.com/juju/juju/pull/7035/files#diff-7> (41) > > Patch Links: > > - https://github.com/juju/juju/pull/7035.patch > - https://github.com/juju/juju/pull/7035.diff > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <#7035>, or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAMYfXzaupn6hjAQZMa8ope4HumD3hIKks5rf50egaJpZM4ML4in> > . >

[anastasiamac]

@jameinel
For normal user management, users should be enabled/disabled rather than removed as per the comments in the bug.
For user removal, the behavior is different - we do not destroy entities that user owns or created when the user is removed: models, applications, etc. This means that if another user with the same name may be allowed to be added, this user will gain access to all these left-over components. Effectively, if we allow the "resurrection" of removed users, the behavior will be surprising :)
As a result, we have decided to disallow the reuse of user name once the user is removed for the life of the controller.

[jameinel]

a) We at least need a proper way to actually resurrect/restore that user. b) How is that going to work with any long-lived controller after a couple of years. Certainly someone is going to end up with a name collision eventually. I'm especially thinking about very large controllers that are intended to interact with very many users.

…

On Mon, Feb 27, 2017 at 3:24 AM, anastasiamac ***@***.***> wrote: @jameinel <https://github.com/jameinel> For normal user management, users should be enabled/disabled rather than removed as per the comments in the bug. For user removal, the behavior is different - we do not destroy entities that user owns or created when the user is removed: models, applications, etc. This means that if another user with the same name may be allowed to be added, this user will gain access to all these left-over components. Effectively, if we allow the "resurrection" of removed users, the behavior will be surprising :) As a result, we have decided to disallow the reuse of user name once the user is removed for the life of the controller. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7035 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMYfQUCZOqhgnsCS9nfoAPPgeYeLx_kks5rggm9gaJpZM4ML4in> .

[anastasiamac]

As far improving messages is concerned, the proposal improves them :D
We'd need to address remove/resurrect users as a separate follow-up

[reedobrien]

$$merge$$

[jujubot]

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju