feat(cmd-register): prevent replacing existing controller if logged in #18079
Conversation
9 similar comments
|
@ca-scribner is this ready for review? |
|
No sorry, I've got a suggestion from @wallyworld I'll be putting in this week. I'm following up with @wallyworld once its ready |
ca-scribner
force-pushed
the
lp2073741-block-users-from-registering-over-their-current-controller
branch
from
c5ed115
to
b8b64c9
Compare
Previously, users could do the following: On computer 1: ``` juju add-user u1 juju add-user u2 ```` On computer 2: ``` juju register TOKEN_FROM_u1 >> works juju register TOKEN_FROM_u2 --replace >> partially replaces the existing controller, but then fails to listModels >> on a permission error because its macaroon chain has a mix of u1 and u2 credentials ``` A similar example with more explanation is in[lp2073741] (https://bugs.launchpad.net/juju/+bug/2073741?comments=all). This was caused by the client chaining macaroons for both user1 and user2 during the API calls, leading to permission errors. The `--replace` flag was implemented to swap users and does not cover this case. This commit prevents the above situation by: * blocking users from `--replace`ing an existing controller they're logged into. Instead, the users is prompted with an error and instructions for resolution. * avoiding invaliding the user's registration token in some cases by adding validation to detect controller collisions before using the token. This is done through updating `ensureNotKnownEndpoint()` to apply its checks to all address types. Prior to this commit, register would attempt to catch duplicate controllers by comparing the new controller's address to all known controllers, but this check was only applied for CLI calls made with a "public host" (a host provided as a URL or IP in the `juju register URL` call). This check was not applied to a controller defined by a base64 token. The present commit updates this check to apply to both address types As a driveby, this commit also: * refactors when we ask the user for a new password. Previously, we asked for a new password prior to doing controller collision checks, resulting in users entering passwords unnecessarily * fixes a mistake in the error message provided on controller collision during `ensureNotKnownEndpoint`. Previously, this message instructed the user to generate a new token prior to trying again, but this is unnecessary as we haven't yet used/invalidated the existing token
ca-scribner
force-pushed
the
lp2073741-block-users-from-registering-over-their-current-controller
branch
from
b8b64c9
to
3eba8be
Compare
| @@ -794,6 +815,51 @@ func genAlreadyRegisteredError(controller, user string) error { | |||
| return errors.New(buf.String()) | |||
| } | |||
|
|
|||
| var alreadyRegisteredMessageTokenStillValidT = template.Must(template.New("").Parse(` | |||
| This controller has already been registered on this client as "{{.ControllerName}}". | |||
| To login user "{{.UserName}}" run 'juju login -u {{.UserName}} -c {{.ControllerName}}'. | |||
There was a problem hiding this comment.
Existing wording, but
"To login as user ..."
here and above etc
| } | ||
| return errors.Trace(err) | ||
| } | ||
| // TODO: Is this check necessary? Can AccountDetails successfully return without setting .User? |
There was a problem hiding this comment.
Haven't we already does this check in the ensureNotKnownEndpoint method?
// Check if we know the username for this controller
accountDetails, err := store.AccountDetails(existingName)
if err != nil && !errors.IsNotFound(err) {
return errors.Trace(err)
}
if accountDetails != nil {
return genAlreadyRegisteredTokenStillValidError(existingName, accountDetails.User)
}
The user may have been prompted for a different controller name to use, but the underlying associated between controller and user (based on ip address match) would have been checked already right? If there is a need for this same check in 2 different places, it seems the existing logic might be a bit convoluted and would benefit from a little extra refactoring. ie can we gather all the inputs needed for the logic up front; am i logged in, does the ip address exist already, is the controller name in the token matching something locally etc. Then the logic can be written a bit cleaner perhaps.
There was a problem hiding this comment.
The check here is similar to the one in ensureNotKnownEndpoint, but we have more information now and can do a more robust check. ensureNotKnownEndpoint compared by address to decide if we already know that controller (which could change over time), whereas here we compare UUID. So this check is (slightly) wider than the previous one.
The need for two checks comes from how we cannot get the controller UUID unless we log into the controller, and that requires burning the user's registration token. So we can't collapse the two checks into one and preserve the user's token on most collisions.
| @@ -435,6 +443,21 @@ func (c *registerCommand) updateController( | |||
| if !c.replace || controllerName != name { | |||
| return genAlreadyRegisteredError(name, accountDetails.User) | |||
| } | |||
|
|
|||
| // We are replacing the current controller - reject the request if this client is logged into that controller | |||
There was a problem hiding this comment.
Nit - use "." at the end of comments.
This PR adds additional checks to
juju registerto block users from adding a known controller. The changes proposed here:Checklist
Integration tests, with comments saying what you're testingdoc.go added or updated in changed packagesQA steps
On computer 1 (with a juju controller):
On computer 2 (without a juju controller):
Documentation changes
Minor change in CLI help
Links
Launchpad bug: https://bugs.launchpad.net/juju/+bug/2073741