Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Return appropriate controller-access for remote users. #6372
Conversation
wallyworld
requested changes
Oct 4, 2016
Maybe we need to talk a bit more about the group permissions
| + // It is possible that there isn't a controllerUser for the remote user, | ||
| + // so in that situation, we allows the remote user login access. | ||
| + if permission.LoginAccess.GreaterControllerAccessThan(controllerAccess) { | ||
| + controllerAccess = permission.LoginAccess |
wallyworld
Oct 4, 2016
Owner
This doesn't look right. And the new tests seems to confirm that. If a permission has been granted to everyone@external, and then that's what will be used for external users (if greater than explicit permission)
There's already a helper for this apiserver/common/maybeUseGroupPermission()
wallyworld
Oct 4, 2016
Owner
Discussed live: as of now, permissions are expected be defined explicitly, either for the specific user, or via the special "everyone@external" group. So if there is no controller access record for the external user, and there is no "everyone@external" permission to fall back on, then we should return a permission denied. We can loosen that strict approach if needed, but right now it's best to start as restrictive as possible. All the current workflows that I am aware of will work with this strict approach.
| + assertPermissionDenied(c, err) | ||
| +} | ||
| + | ||
| +func (s *loginSuite) loginUserWithAccess(c *gc.C, info *api.Info) (*state.User, params.LoginResult) { |
wallyworld
Oct 4, 2016
Owner
what does "WithAccess" mean here?
It seems like it should mean "log in this specified user with this specified access" but that doesn't seem to be the case?
|
@wallyworld updated the behaviour, extracted into its own function. |
| c.Check(result.UserInfo.ModelAccess, gc.Equals, "write") | ||
| } | ||
| +func (s *macaroonLoginSuite) AddControllerUser(c *gc.C, user names.UserTag, access permission.Access) { |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
howbazaar commentedOct 4, 2016
This bug addresses two issues:
http://pad.lv/1621679 - Login succeeds when permissions forbid it but all requests are denied afterwards
http://pad.lv/1629089 - API Login ACL response values differ from CLI
The authenticiation code was not adding higher level "everyone" controller access to the login response.
Also, the same area of code would not reject access to a model for an unknown user if there was an "everyone" controller permission.
Tidied up the tests a bit to use test cleanup to close api connections rather than adding a defer call to every test.