Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Improve ACL testing. #6384
Conversation
| @@ -59,7 +59,7 @@ func (s *controllerSuite) SetUpTest(c *gc.C) { | ||
| AdminTag: s.Owner, | ||
| } | ||
| - controller, err := controller.NewControllerAPI(s.State, s.resources, s.authorizer) | ||
| + controller, err := controller.NewControllerAPI(s.State, s.resources, &s.authorizer) |
| @@ -638,6 +638,15 @@ func (s *controllerSuite) modifyControllerAccess(c *gc.C, user names.UserTag, ac | ||
| return result.OneError() | ||
| } | ||
| +func (s *controllerSuite) TestAddModelCantGrant(c *gc.C) { |
wallyworld
Oct 10, 2016
Owner
So this is testing that a model owner cannot grant add-model access to a controller?
That seems a bit narrow - the only users who can grant add-model access to a controller are users with superuser access on the controller. I'm sure there are tests somewhere that check this matrix. I am not sure the point of adding a single extra test here.
perrito666
Oct 11, 2016
Contributor
it actually checks that someone with addmodel cant grant permissions on a given controller, I think we had this bug earlier, that is why I added it.
| @@ -468,6 +468,15 @@ func (st *mockState) RemoveModelUser(tag names.UserTag) error { | ||
| func (st *mockState) UserAccess(tag names.UserTag, target names.Tag) (permission.UserAccess, error) { | ||
| st.MethodCall(st, "ModelUser", tag, target) | ||
| + for _, user := range st.users { | ||
| + if user.UserTag == tag { |
wallyworld
Oct 10, 2016
Owner
we could save a level of indentation by inverting the if and using continue
| + return m.getModelInfoPluggableAuth(tag, m.authCheck) | ||
| +} | ||
| + | ||
| +func (m *ModelManagerAPI) getModelInfoPluggableAuth(tag names.ModelTag, |
wallyworld
Oct 10, 2016
Owner
I don't see what this pluggableAuth change is for. The only functional difference I can see is that we are passing in something that always return nil when called by CreateModel. Why isn't the current authCheck sufficient in that case?
perrito666
Oct 11, 2016
Contributor
because, touching authcheck at this point might have undetectable side effects since iirc its being used by more than one places.
perrito666
Oct 11, 2016
Contributor
because authcheck is a generic little thing that is used a few times and its not the purpose of this PR factor it out in favor of a modern aproach to the check so I wont change auth check and risk un-intented collateral behaviors that could be potentially untested.
wallyworld
Oct 12, 2016
Owner
I think my point is that the current authCheck code should be sufficient without any changes needed
| @@ -42,8 +42,21 @@ import ( | ||
| type modelManagerBaseSuite struct { | ||
| } | ||
| +func (s *modelManagerBaseSuite) createArgs(c *gc.C, owner names.UserTag) params.ModelCreateArgs { |
wallyworld
Oct 10, 2016
Owner
this method doesn't need to be on a suite - it can just be a stand alone helper
it also doesn't use the c *gc.C arg
| type modelManagerSuite struct { | ||
| gitjujutesting.IsolationSuite | ||
| + modelManagerBaseSuite |
wallyworld
Oct 10, 2016
Owner
you can't embed a new suite without adding the necessary SetUp and TearDown funcs to call into IsolationSuite and modelManagerBaseSuite
perrito666
Oct 11, 2016
Contributor
modelManagerBaseSuite is no longer embeded so no need to write the methods.
| @@ -590,10 +603,50 @@ func (s *modelManagerSuite) TestDumpModelsDBUsers(c *gc.C) { | ||
| } | ||
| } | ||
| +func (s *modelManagerSuite) TestAddModelCanCreateModel(c *gc.C) { |
wallyworld
Oct 10, 2016
•
Edited 1 time
-
wallyworld
Oct 10, 2016
Owner
-
Oct 10, 2016
Is there a test that users without add-model access cannot add a model? This test really belongs next to that.
We already have
TestUserCanCreateModel
TestAdminCanCreateModelForSomeoneElse
TestNonAdminCannotCreateModelForSomeoneElse
etc
These belong next to those. It may be getting to the point where a table test with {user, access, allowed} is a better approach
| @@ -590,10 +603,50 @@ func (s *modelManagerSuite) TestDumpModelsDBUsers(c *gc.C) { | ||
| } | ||
| } | ||
| +func (s *modelManagerSuite) TestAddModelCanCreateModel(c *gc.C) { | ||
| + addmodel := names.NewUserTag("addmodel@local") |
| @@ -590,10 +603,50 @@ func (s *modelManagerSuite) TestDumpModelsDBUsers(c *gc.C) { | ||
| } | ||
| } | ||
| +func (s *modelManagerSuite) TestAddModelCanCreateModel(c *gc.C) { | ||
| + addmodel := names.NewUserTag("addmodel@local") |
| + return false, nil | ||
| +} | ||
| + | ||
| +func nameBasedHasPermission(name string, operation permission.Access, target names.Tag) bool { |
wallyworld
Oct 10, 2016
•
Edited 1 time
-
wallyworld
Oct 10, 2016
Owner
-
Oct 10, 2016
this really needs a comment to explain why it is needed
it seems like it would remove the need for HasWriteTag
|
@wallyworld ptal I added a few changes |
perrito666
changed the base branch from
master
to
develop
Oct 14, 2016
|
!!try!! |
|
!!retry!! |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
The build failed the ACL functional test. It did the following: create a read-only user for a model and register them When the test attempted to login again, it did not prompt for a password as it should as the user should no longer be logged in. That would be a problem :-) |
|
!!try!! |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
perrito666 commentedOct 5, 2016
•
Edited 1 time
-
perrito666
Oct 5, 2016
addmodel permission instead of superuser.
This fixes https://bugs.launchpad.net/juju/+bug/1618966
QA Steps